feat(test): Miscellaneous improvements to test_tasks.js#6615
Merged
cpcallen merged 3 commits intoNov 16, 2022
Conversation
github-actions
Bot
added
PR: feature
BeksOmega
requested review from
BeksOmega
and removed request for
rachel-fenichel
BeksOmega
reviewed
Nov 14, 2022
BeksOmega
left a comment
BeksOmega
left a comment
Contributor
There was a problem hiding this comment.
Slightly confused about the decision to make a Tester class. Otherwise generally LGTM, but want to understand that before approving.
| const ANSI_RESET = '\x1b[0m'; | ||
|
|
||
| let failerCount = 0; | ||
| class Tester { |
Contributor
There was a problem hiding this comment.
What's the advantage of encapsulating these in a class?
Collaborator
Author
There was a problem hiding this comment.
Mainly just to remove the failerCount global variable, and allow us to also count successes without adding a second global.
Modify runTestBlock so that it can run any async task, not just ones that return a Promise, by using the async-done package (part of Gulp, and already an indirect dependency) to detect task completion. Celebrate by renaming it to runTestTask.
- Create Tester class to encapsulate the runTestTask, and reportTestResult and runAll functions. - Remove the unnecessary id parameter from runTestTask (code was already using the .name of the task function object). - Remove --silent flag from npm scripts so as not to suppress syntax error in gulpfiles.
Have the test task invoke the buildAdvancedCompilationTest (via onlyBuildAdvancedCompilationTest, to skip already-run prerequisites) directly, rather than by running npm.
cpcallen
force-pushed
the
improve-test-tasks
branch
from
b8da89a to
868cc74
Compare
Collaborator
Author
|
There were merge conflicts, and it seems like I forgot a needed Just awaiting approval, assuming you think my justification for the |
BeksOmega
approved these changes
Nov 16, 2022
This was referenced Jan 11, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The basics
npm run formatandnpm run lintThe details
Proposed Changes
runTestBlockso that it can run any async task, not just ones that return aPromise, by using theasync-donepackage (part of Gulp, and already an indirect dependency) to detect task completion. Celebrate by renaming it to runTestTask.Testerclass to encapsulate therunTestTask,reportTestResultandrunAllfunctions.--silentflag from npm scripts so as not to suppress syntax error in gulpfiles.testtask invoke thebuildAdvancedCompilationTesttask (viaonlyBuildAdvancedCompilationTest, to skip already-run prerequisites) directly, rather than by runningnpm.Reason for Changes