Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for CLI generators #401

Merged
merged 34 commits into from
Apr 10, 2020
Merged

Conversation

peterp
Copy link
Contributor

@peterp peterp commented Apr 7, 2020

No description provided.

@thedavidprice
Copy link
Contributor

thedavidprice commented Apr 7, 2020

Babel ignore …test.js.snap and __snapshot__/

@peterp I spend a good amount of time jiggering our Babel ignore config — at this point all I know is that something is broken, but I have no idea what, specifically.

Here is the build script:

"build": "yarn cross-env NODE_ENV=production babel src -d dist --delete-dir-on-start --copy-files --no-copy-ignored",

And the associated configuration for ignored files, which I am attempting to update to include **/__snapshots__ and files matching …test.js.snap:

redwood/babel.config.js

Lines 89 to 92 in cc7b342

ignore:
process.env.NODE_ENV === 'production'
? [/\.test\.(js|ts)/, '**/__tests__', '**/__mocks__']
: [],

Here are my results:

  • the current ignore patterns/settings are NOT working in full. The only pattern that is ignored is that for …test.js files.
  • I tried altering /\.test\.(js|ts)/ to only specify .ts for testing, however, babel would not ignore .ts (I created a test test file). Additionally, babel is not currently ignoring the directories — if it were, the test file patterns wouldn’t be needed in the cli package. (I did try the directory matching for each individually, using different options, and couldn’t get them to work.)
  • I tried adding —ignore **/__tests__ to the script, which did not work
  • I tried adding the ignore: [..] config to cli/.babelrc.js, which did not work

My suspicion is that something is broken between using “ignore: []” within the config file and “--no-copy-ignored” within the script. However, this still doesn’t explain how the “ignore: []” does work for the /\.test\.(js|ts)/ setting but for nothing else. E.g. it does not ignore “..test.ts” file if I try only /\.test\.ts/

JS, am I right?!? ¯_(ツ)_/¯

Edit: Oh yeah, docs… here’s what I was referring to :

@thedavidprice
Copy link
Contributor

My best diagnoses is that babel’s new --no-copy-ignored is busted — it only ignores .js files but doesn’t work with anything else in the config “ignore:” array. See this comment:
babel/babel#6226 (comment)

I submitted a new issue:
babel/babel#11394

Now trying an alternate method — possibly a second script for cleanup of “dist/“ after building.

Babel flag --no-copy-ignored has a bug and does not respect our ignored config. For a short term fix, I'm using rimraf package (cross-platform support) to clean up __tests__ folders that shouldn't be in dist/
@thedavidprice
Copy link
Contributor

@peterp I believe this is ready for you to review.

I’m using a workaround for the build, which includes a new build script setup and a cross-platform package, rimraf, to clean up the __tests__ directories that shouldn’t ship in “dist/“.

NOTE: I tested the cli build locally on my Windows setup. The build script runs correctly with no errors. However, the rimraf command uses a glob and does not work as expected on Windows (the __tests__ directories are not deleted). Since this will only affect Windows local build and dev, and is not an issue with “dist/“ but merely un-optimal for publishing, I suggest we proceed as is.

@cannikin might help to have your eyes on the build “dist/“ one more time before we merge.

@cannikin
Copy link
Member

cannikin commented Apr 9, 2020

@thedavidprice /dist looking good!

@peterp I think this is ready if you want to review the 101 files that changed. If that's too much let's get on a call and I can go through them.

image

I didn't go with snapshots, I ended up making a /fixtures directory inside each __tests__. Having that one giant snapshot that had to be updated seemed fragile to me: you could have made a change that broke multiple tests, you fix one test and then update—now all the snapshots are updated but you never fixed the other tests. They're passing and you assume everything is good. WHOOPS. I saw a fixtures path in one of the other packages so it didn't feel too crazy. Now you can be fairly confident that if you update a fixture it will only affect the tests that directly reference it.

@cannikin
Copy link
Member

cannikin commented Apr 9, 2020

Heads up, it might not affect anyone else but I'm going to branch off this branch and work on the new relationships stuff.

@peterp
Copy link
Contributor Author

peterp commented Apr 9, 2020

@cannikin cool, let's chat tomorrow

@cannikin cannikin merged commit 8d510db into master Apr 10, 2020
@cannikin cannikin deleted the pp-add-tests-for-cli-generators branch April 10, 2020 17:16
mohsen1 pushed a commit to mohsen1/redwood that referenced this pull request Apr 12, 2020
* Remove global mocks, localize to test.

* Remove more global mocks.

* Make dbCommand tests work again.

* Write snapshot tests for generators.

* Add generate tests and snaps.

* Play around with snapshot testing.

* Update snapshots.

* Complete reorg of generator files and templates, adds text and fixture directories

Cell test working

* Component, layout, page and service tests

* Adds tests for generator helpers.js

* More tests

* Comment out some variables that aren't being used for now

* Adds multi-word cell tests

* Variable name change

* Adds multi-word component tests

* Component test fix

* Adds multi-word layout tests

* Adds multi-word page tests

* Creates multi-word and CRUD sdl tests

* Adds multi word tests for service

* Adds scaffold tests

* Count actual number of files returned

* Rename test directories to __tests__

* Adds tests for src/lib/index

* Ignore any files named "test" inside a fixtures directory

* Adds generateTemplate tests

* Ignore fixture files when linting

* fix babel build ignore error using rimraf (rm -rf)

Babel flag --no-copy-ignored has a bug and does not respect our ignored config. For a short term fix, I'm using rimraf package (cross-platform support) to clean up __tests__ folders that shouldn't be in dist/

* Converts SDL generator to use same sub-generator technique as scaffold generator

* Updates README with new generator file structure

* Removes .fixture extensions

* Remove unused argument

Co-authored-by: Rob Cameron <rob.cameron@fastmail.com>
Co-authored-by: David S Price <thedavid@thedavidprice.com>
@thedavidprice thedavidprice added this to the unassigned-version milestone May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants