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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure Jest dynamically per side #805

Merged
merged 8 commits into from
Jul 15, 2020

Conversation

RobertBroersma
Copy link
Contributor

@RobertBroersma RobertBroersma commented Jul 8, 2020

馃憢

This works, but only by circumventing an alleged bug in Jest.


A simpler way to do this would be to require the user to define a jest.config file for every side that they create. This could be generated by create-redwood-app or by yarn rw generate side (or w/e @peterp has in store for us!). This way the main config for the test command can be simplified to just pick up any project (jest.config) inside of the user's monorepo.

The file contents could be defaulted to something like

const { createBrowserJestConfig } = require('@redwoodjs/core')

module.exports = createBrowserJestConfig()

Upsides to this simpler approach:

  • No Jest overrides
  • The generated jest.config files provide an excellent entry point for extending configs.
  • In the current setup of this PR, it does work out of the box in current RW projects and future projects, but it's highly likely (assumption) that most users will want to extend their Jest config at some point. So they'll have Jest Configs in their project anyway. We would then be supporting 2 approaches, only to avoid requiring config files for existing RW apps.

The biggest downside to that would be that existing projects would have to add these configs manually.

@peterp @thedavidprice @ others, what do you think? Typing this all out pretty much convinced myself the simpler approach is better, but I'd love to hear what you think 馃槃


Benefits:

  • Nice colors in CLI.
  • Run watch simultaneously over every Side.
  • Full coverage for all your code in one place.
  • Use watch plugins globally (future stuff).

TODO Before merge:

  • Decide approach.
  • Expose Jest side configs for extending.
  • Dynamically add colors for different sides.

@Tobbe
Copy link
Member

Tobbe commented Jul 8, 2020

I prefer the simpler approach as well. And I wonder, could we make yarn rw upgrade add default jest.config files if they don't exist?

@RobertBroersma
Copy link
Contributor Author

@Tobbe Oooh that sounds like a good idea!

@peterp
Copy link
Contributor

peterp commented Jul 9, 2020

@RobertBroersma I like your idea of adding the configuration file to a project; and I really like the way that you've got a function to grab that config, what do you think of making it more generic?

const { getConfig }  = require('@redwoodjs/core')
module.exports = getConfig({ type: 'jest', target: 'browser' })

@RobertBroersma
Copy link
Contributor Author

I think that's a good idea, @peterp !

@RobertBroersma
Copy link
Contributor Author

I've updated this with the simpler approach as discussed!

Check this repo for the jest.config.js's (In the web and api folders): https://github.com/RobertBroersma/redwood-testing
(I think it's been mentioned before, but it might be nice to have some testing repo inside the monorepo to see the changes alongside potential changes required in apps + for testing!)

Does this getConfig kind of fit what you had in mind, @peterp ?

I'll do a short writeup of some assumptions/default config I did in the config functions and in the test command later!

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really struggling to see the value versus the complexity introduce via this PR.

packages/cli/src/commands/test.js Outdated Show resolved Hide resolved
packages/cli/src/commands/test.js Show resolved Hide resolved
packages/cli/src/commands/test.js Outdated Show resolved Hide resolved
packages/cli/src/commands/test.js Show resolved Hide resolved
packages/cli/src/commands/test.js Outdated Show resolved Hide resolved
packages/core/config/babel-preset.js Outdated Show resolved Hide resolved
packages/core/config/jest.config.js Show resolved Hide resolved
@RobertBroersma
Copy link
Contributor Author

I'm really struggling to see the value versus the complexity introduce via this PR.

Can you elaborate on what you find more complex in this situation? In my mind I just moved a few things around and made it less complex! Perhaps I can shine light on some stuff or maybe I'm in too deep.

@peterp
Copy link
Contributor

peterp commented Jul 9, 2020

@RobertBroersma The stuff related to the type of VCS you're using really threw me off, I'll take another look with some fresh eyes in the morning.

@RobertBroersma RobertBroersma marked this pull request as ready for review July 15, 2020 08:31
@peterp
Copy link
Contributor

peterp commented Jul 15, 2020

It looks good to me! We just need to confirm that the changes to using the absolute paths for the main babel src alias isn't breaking deployments. (I left a note.)

@RobertBroersma
Copy link
Contributor Author

It looks good to me! We just need to confirm that the changes to using the absolute paths for the main babel src alias isn't breaking deployments. (I left a note.)

My bad! Can be removed. Thought I did.

@RobertBroersma RobertBroersma merged commit f5bcd54 into redwoodjs:main Jul 15, 2020
@Tobbe
Copy link
Member

Tobbe commented Jul 15, 2020

I've been reading about paths, which always gets my windows spidey senses tingling... Want me to test-run anything on win?

@RobertBroersma
Copy link
Contributor Author

@Tobbe If you can run yarn rw test with this little test repo: https://github.com/RobertBroersma/redwood-testing to confirm that would be magnificent!

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

4 participants