Conversation
275cc40
to
4e8adca
Compare
the flaky test here in models is fixed by #2943 (missing awaits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good setup! I like the consistency of having a top-level test suite, rather then some random bash scripts we run.
I think there are a few topics to look into:
- Don't import/require anything from grouparoo/core or grouparoo/spec-helper. They shouldn't be needed and I'm worried that will have an adverse effect on the publishing pipeline
- Can JSON-schema assert the contents of the JSON rather than the types? For example, we don't really care that publish config is /any/ string in the packages, we really do want to assert that it is "public". I think that's a better reflection of the goal.
__tests__/packages.ts
Outdated
if (error || stderr) { | ||
reject(error || stderr); | ||
} | ||
res(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below - an early reject without a return will still also resolve later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing done(error || stderr)
and not wrapping all this in a promise may be the best way to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with the done()
pattern for both the version and license checkers and piped out the most meaningful messaging for each checker (license checker shows it in stderr
, but the version checker shows it in stdout
)
1ae84ea
to
99820ce
Compare
ba97faa
to
8023153
Compare
Changes since last review:
|
4b00000
to
34d2a0d
Compare
6cfa4be
to
5c407d4
Compare
Change description
Went with invoking the version checker and spelling checker within child process inside the jest test as the scripts in place did a more efficient job of the tasks then a JavaScript solution would in my opinion.
Also removed the CI jobs that used to execute those scripts, and added one to run the new suite.
Checklists
Development
Impact
Please explain any security, performance, migration, or other impacts if relevant:
Code review