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

[Testing] Improve testing capabilities for Web #521

Merged
merged 28 commits into from
May 28, 2020

Conversation

RobertBroersma
Copy link
Contributor

@RobertBroersma RobertBroersma commented May 8, 2020

related to #502

This PR is for writing tests in the web side!

Routes

Normally when using @reach/router or react-router I wouldn't mock my <Router /> component, I would just wrap the render in it, but @redwoodjs/router makes that difficult because

  • <Router /> doesn't accept children.
  • <Router /> does some initial magic with pages that you probably don't want in your test.

I did 2 things to "solve" these issues:

1. Import the Routes.js file from the application to the Redwood library.

This line in jest.config.web.js does exactly that. <rootDir> is the application root.

import Routes from '~/web/Routes': '<rootDir>/src/Routes',

By rendering the <Routes /> component before the subject, all routes should be calculated before they are used in the subject.

2. Mock <Router />

I can't quite figure out why, but I had some difficulties with page-loader.js when not mocking <Router />. In order for any of this to work I put this mock in __mocks/@redwoodjs/routes.js:

export * from "@redwoodjs/router";

const routes = {};

export const Router = ({ children }) => {
  for (let route of React.Children.toArray(children)) {
    const { name } = route.props;
    routes[name] = jest.fn(() => name);
  }

  return <></>;
};

export { routes };

This still enables using routes.nameOfRoute in your tests, but mocks out the rest, meaning nothing of <Routes /> or <Router /> will also show up in your tests!

I'm not sure where this mock should live. I think keeping all mocks on the application side could be the best, so there's no hidden differences in application and test going on for the user. It might also be difficult to have this mock live inside node_modules, Jest doesn't deal with that.

One thing we could do is keep it in node_modules and import it inside the application mock.

Apollo MockProvider

Before we come up with some fancy tool that knows about your backend and can assist you in your tests, I think the best would be to just support mocking out the backend. This is pretty straightforward to do with Apollo's MockProvider!

Implementation

I've created a small repo that demonstrates the usage of this PR: https://github.com/RobertBroersma/redwood-testing

I went through about half of the tutorial to get some Cells. web/src/pages/BlogPostPage/BlogPostPage.test.js contains all the stuff I mentioned!

Conclusion

It works, but it's still WIP! As always happy to receive any feedback.

One bullet would be where to keep all these files? I suppose some of that jest config could be moved to testing package? Could the testing package be small enough to merge into core? Would it grow in the future?

PS. Please ignore the babel cell plugin stuff. It's in a separate PR #512 . I need that for Jest to work with Cells. Feel free to ignore this until #512 is merged!

@peterp peterp force-pushed the master branch 6 times, most recently from 711c520 to 2341368 Compare May 10, 2020 10:07
packages/testing/src/render.js Outdated Show resolved Hide resolved
packages/testing/src/render.js Show resolved Hide resolved
@cball
Copy link
Contributor

cball commented May 11, 2020

@RobertBroersma awesome work! re: your point on 2. Mock <Router />, that's something you'd typically have to do when using next.js as well.

I like your idea of automatically handling the mock in Redwood and maybe in the future the mocked jest.fn could be exposed so that users could assert that it was called with the correct parameters.

@RobertBroersma
Copy link
Contributor Author

@cball Yeah I've been checking out Gatsby docs as well and they recommend mocking out Gatsby entirely 😄

Thanks again for the feedback. Like I said it's very helpful to get some perspective from someone with actual experience using this stuff!

@RobertBroersma RobertBroersma marked this pull request as ready for review May 17, 2020 07:56
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.

@RobertBroersma This is amazing! Your contributions are always solid and I really appreciate it!

@@ -0,0 +1 @@
module.exports = 'fileMock'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this file could be part of @redwoodjs/testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it could. Do you think it would be a good idea to move other testing stuff like Jest config and the setup files to that @redwoodjs/testing as well? And have the testing package sort of dogfood the rest of the monorepo?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to keep configuration in core. In my mind the "core" package is what a user's requires to develop and build a redwood project and the configurations are part of that.

The file mock feels like something that should belong to test, whereas this configuration is something that's using "@redwoodjs/testing" - that distinction may seem a bit loosey-goosey and I'll try to clarify it with better language.

packages/core/config/fileMock.js Outdated Show resolved Hide resolved
packages/core/config/jest.config.web.js Show resolved Hide resolved
packages/core/config/jest.config.web.js Outdated Show resolved Hide resolved
packages/testing/src/render.js Outdated Show resolved Hide resolved
packages/testing/src/index.js Show resolved Hide resolved
packages/testing/src/render.js Show resolved Hide resolved
@peterp
Copy link
Contributor

peterp commented May 17, 2020

@cball Thank you for chiming in here too! You made me consider a bunch of things that wasn't really in my mind. Much appreciated!

@RobertBroersma
Copy link
Contributor Author

@cball We went ahead and removed MockedProvider from the default render export until we come up with an API that's certain to satisfy all!

@RobertBroersma
Copy link
Contributor Author

(I'm not entirely sure what's wrong with my build checks)

@thedavidprice
Copy link
Contributor

@RobertBroersma this is looking so. good. Just 🔥Some misc topics for discussion as you are nearing the end!

Documentation

In general, we're trying to create a distinction between docs for 1) contributing to Redwood and 2) developing with Redwood. The former is the content in package READMEs, and the latter is the Guides section of redwoodjs.com and lives in the related repo docs/ directory. Here's my current (very much draft) attempt to add some guidelines #332

For the docs you've started here, can we:

  • move the "developing" related content to a new Issue on redwoodjs.com repo
  • use the README stub structure (from 332) to create the README here?

I'll help with any/all of this. Also looping in @jtoar Just say the word!

Jest --config extensibility

I want to keep this future requirement on the radar (note: it's definitely not a "must include" in this PR).

if additional config is needed, capability for simple config extension managed from either App root or specific to App sides (e.g. web/, api/, etc.).

The current design of the yarn rw test command along with jest config in the core package makes it non-intuitive how to extend/add config if needed.

Robert, if you already have an idea about how this could work, near-term I could help give some guidance via the Testing Docs (if possible). Otherwise, we'll get to this down the road but let's not lose track of it.

Updating the RW Generator .test.js templates

Our goal for Redwood's testing support is that individuals will be able to go through the Tutorial and end up with passing tests along with the app they created.

Our current test templates will need to be updated to match the work you've done here. Just point us in the right direction when you're ready and we'll make sure it happens!


Thanks again!

@RobertBroersma
Copy link
Contributor Author

@thedavidprice Thanks for the input!

For the docs you've started here, can we:

move the "developing" related content to a new Issue on redwoodjs.com repo
use the README stub structure (from 332) to create the README here?

✔️ Will do! I'll tag you and @jtoar when I do to make sure it's all in the right place and the language is good.

Jest --config extensibility

Important to keep on the radar indeed! Atm you could extend the config by directly importing the jest config from core and merging it with your local jest config, however this is

  1. Currently undocumented
  2. Not optimal...? I'm thinking along the way of exposing the Redwood jest config(s) as a function that you could call. Something like:
import { createJestConfig } from '@redwoodjs/core' (or @redwoodjs/testing)

export default createJestConfig({
 ...myCustomStuff,
})

(As there is no extend feature in Jest like in Babel/Eslint)

Let's create a separate GitHub issue to continue discussing this? 🚀

About targeting different sides; I thought it could be good to use Jest's [projects](Jest --config extensibility) config instead of running all sides in the command, so we offload more stuff to the Jest side, and new sides with their own jest.configs are automatically picked up!

One more thing I'd also like to do in the future is add https://github.com/jest-community/jest-watch-typeahead to the base config. This of course requires yarn rw test to work with watch!

Updating the RW Generator .test.js templates

I think we for sure should Include the __mock__/@redwoodjs/router mock I haphazardly documented in the README on this PR by default. This would be required to get the tests with routes.x() in them to work

Another thing I think we should do is change the generated tests for Cells (Partially like in the README on this PR + Some extra tests for Failure and Empty). This is however up for debate, as merging this won't break the current test setup. The new suggested setup would just push the tests more from unit tests to integration tests (if we'd have to slap a label on it).

Let me know if you'd like me to create some of those new issues I mentioned.


Cheers!

@thedavidprice
Copy link
Contributor

thedavidprice commented May 19, 2020

Documentation --> perfect. And don't hesitate to offload as needed!

Jest --config --> Done #564 Ok by me to start with the "documenting" option. Maybe add to current round of docs?

Templates --> agreed on both counts for Pages and Cells (we could definitely use better template code for Cells). I'll assume you will take lead, but I’m happy to help if you let me know!

@RobertBroersma
Copy link
Contributor Author

Thanks @peterp !

@thedavidprice I added a very concise implementation of the Readme.md stub you linked. Feel free to add more details if you have the time! I wasn't sure who to put for package lead. I put myself for now, but maybe that should be someone from the core team? 😄

I have some small changes to the mock instructions since last time. I will open up a PR on redwoodjs.com repo asap :)

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.

Hey @RobertBroersma I'm trying to get this to run in example-blog, but it doesn't appear to be able to import the pages correctly.

Did you have an example app where you got this working?

@RobertBroersma
Copy link
Contributor Author

Hey @RobertBroersma I'm trying to get this to run in example-blog, but it doesn't appear to be able to import the pages correctly.

Did you have an example app where you got this working?

My bad! I had some small changes to the example, forgot to push them. If you pull on master it should all work.

@RobertBroersma
Copy link
Contributor Author

@peterp I see now that you mentioned example-blog, not my redwood-testing repo! Did you take a look at that one for the web/__mocks__/@redwoodjs/router.js mock file? That is a requirement to get the routes.route() functions to work. I will open a doc PR and a PR for generating thing file asap!

@peterp peterp merged commit 40277e3 into redwoodjs:master May 28, 2020
@peterp peterp added this to the next release milestone May 28, 2020
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.

4 participants