Conversation
c44abb7 to
c6ce63d
Compare
| if (projectName == null) throw new Error('Project name not found'); | ||
| await page.getByRole('textbox', { name: 'Project Name' }).click(); | ||
| await page.getByRole('textbox', { name: 'Project Name' }).click(); | ||
| if (projectName == null) throw new Error('Project name not found'); |
Check notice
Code scanning / CodeQL
Unneeded defensive code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 months ago
To fix the problem, we need to remove the redundant check projectName == null on line 180. Since the projectName variable can only be undefined or a string, we should replace the check with projectName === undefined. This will ensure that the code correctly handles the case where projectName is not found without introducing unnecessary defensive code.
| @@ -179,3 +179,3 @@ | ||
| await page.getByRole('textbox', { name: 'Project Name' }).click(); | ||
| if (projectName == null) throw new Error('Project name not found'); | ||
| if (projectName === undefined) throw new Error('Project name not found'); | ||
| await page.getByRole('textbox', { name: 'Project name' }).fill(projectName); |
There was a problem hiding this comment.
No idea why it's saying this.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3137 +/- ##
=======================================
Coverage 81.98% 81.98%
=======================================
Files 605 605
Lines 34694 34694
Branches 5622 5622
=======================================
Hits 28444 28444
Misses 5411 5411
Partials 839 839 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c6ce63d to
511f1c6
Compare
511f1c6 to
240ee13
Compare
240ee13 to
da62195
Compare
da62195 to
70c712a
Compare
70c712a to
05107a7
Compare
05107a7 to
ee77482
Compare
ee77482 to
a868388
Compare
a868388 to
8f539c4
Compare
8f539c4 to
c7d706b
Compare
Also removed executable flag from smoke-tests.mts
94f49c9 to
e1f38b5
Compare
pmachapman
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Nateowami)
Nateowami
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/e2e/copy-help-site-screenshots.mts line 85 at r8 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Do we need to document somewhere what package to install for this?
I added a note, but didn't add it to the package.json because I haven't figured out how to use it as a library, it's only designed for Unix-like systems, and this script is still work-in-progress. It could be replaced with something like pngcrush, but the performance of pngcrush and anything else I've found doesn't match it.
pmachapman
left a comment
There was a problem hiding this comment.
I'll approve this, and leave for you to merge when you think it is ready (I'm happy with the premise and logic). I still can't get the tests running perfectly at my end, but I think that is a "me" problem.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/e2e/copy-help-site-screenshots.mts line 85 at r8 (raw file):
Previously, Nateowami wrote…
I added a note, but didn't add it to the package.json because I haven't figured out how to use it as a library, it's only designed for Unix-like systems, and this script is still work-in-progress. It could be replaced with something like pngcrush, but the performance of pngcrush and anything else I've found doesn't match it.
I agree.
This change is