Refactor build-default tests for more explicitness and correctness checks #903
Conversation
This file contains 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
- I accidentally put it into build-default before build-options existed, but the test very specifically tests an option, `--target node`, so it should be in build-options as that's not a zero-config default
- the previous console.log statement would actually log out because
one of the tests imports (`require`s) the actual built library in
order to ensure correctness
- during the import, the side-effect runs and prints, which ends up
actually printing out during the test, which was quite noisy 😅
- we do still want a side-effect so the whole statement doesn't get
removed as dead code, so use a global variable assignment instead
- instead of random names (which are bad practice), be very explicit
about what the variables _should_ be
- also add comments to some of the regression tests that didn't have
any to describe them
- similarly, when testing dev only logging, say that directly
- and, in this case, instead of using an unnecessary swear word
- the third contributor PR to TSDX removed some similar swear words
in the templates
- these were changed to "boop" and now similarly changed to be
more descriptive
- which is better practice anyway, not encouraging bad practices
via the templates, and should also be easier to understand for
newcomers
- similarly, intead of "blah" and "works" in template tests, actually
describe what the tests do
- similarly encourage better practices in the tests
- also rename template tests from "blah.test.ts" to "index.test.ts"
because they test "index.ts" and it's common convention to name them
the same
- and fix a leftover "describe('it')" in the react template when it
should be "describe('Thing')" which the storybook template has
- "it" is also confusing when used together with the Jest "it" global
- could use something better than "Thing" as well (e.g.
"SomeComponent"), but don't want to dig through and re-test the
Storybook pieces right now
- it uses normal .ts files and no JSX (that's what the React template is for after all), so README shouldn't mention .tsx - woops, this was probably a copy+paste error on my behalf when I was improving the basic template's docs
- have most of the syntax tests export a function that returns true
if the syntax is evaluated correctly, and test that it does in fact
return true
- just using `true` as something consistent and easy to test for
- change the async test to move the side effect before the Promise
resolves
- not entirely sure why, potentially because of the way the event
loop works (race condition), but this was returning false when
after and true when before
- both work as a side effect, so just have it before
This comment has been minimized.
This comment has been minimized.
agilgur5
added
scope: internal
agilgur5
commented
Oct 14, 2020
There was a problem hiding this comment.
LGTM. A lot of files changed, but mostly one-liners. Biggest change was moving the regenerator test to build-options.
Not sure why Windows test on Node 10 is failing, one of the integration tests for error extraction (which doesn't totally work, so is only a partial test) failed, but that wasn't changed (only build-default was). re-running..
|
Test passes now, not sure why it failed that one time. First time I've seen that error and it's in a section of the test suite that I didn't change and a not so important test, so going to merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Summary by Commit Message
refactor: move unbundled regenerator test to build-options
but the test very specifically tests an option,
--target node, soit should be in build-options as that's not a zero-config default
refactor: use a less noisy side-effect for async regression test
one of the tests imports (
requires) the actual built library inorder to ensure correctness
refactor: be more descriptive than "blah", "foo", "bar" in tests
fix/docs: basic README should reference .ts, not .tsx files
it uses normal .ts files and no JSX (that's what the React template
is for after all), so README shouldn't mention .tsx
woops, this was probably a copy+paste error on my behalf when I was
improving the basic template's docs
This one is unrelated to the rest, just found it as I was refactoring for the previous commit
refactor/test: test for correctness of syntax, not just parsing
if the syntax is evaluated correctly, and test that it does in fact
return true
Tags
build-optionswas only just added with my smoke test in fix/deps: upgrade rollup-plugin-typescript2 to fix cache issue #896, so I must've forgot to split out files before.Review Notes
Been wanting to do the explicitness for a while. Consistency in the syntax testing I thought would help (for my own use writing tests too) and correctness seemed to make more sense and was something I already did with the generator test (so this makes the others consistent in checking for correctness as well). I didn't actually add correctness tests (i.e. checking the value) until now, maybe because I forgot I already had a place in the test suite to do so.
This doesn't change any actual source code or meaningful template code.
Will rebase these in.