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

feat(packages/sui-jest): Allow test server& browser environments #1655

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oriolpuig
Copy link
Contributor

@oriolpuig oriolpuig commented Nov 3, 2023

Description

Current version of sui-jest is passing the responsibility of having a jest.config.js file to each project, instead of follow the usual SUI approach by wrapping a library and unify how as much as projects uses the same configuration.

We have started using jest with component testing, but when we started to use it on sui-domain tests, we needed some customisations to run test on server and client sides. Those customisation, cause some extra project configuration that we can avoid with this Pull Request

With this Pull Request I want to make our life more easier and standardise the default sui-jest configuration through all sui projects. Of course, we also allow each project to pass custom configuration and override the default one.

Some things this Pull Request enables:

  • Avoid duplicated configuration through several projects.
  • Let each project to override default configuration.
  • Enable to use sui-jest browser or sui-jest server to run tests on different environments and it will add extra configuration needed for each environment.
  • Avoid to maintain two tests file. One for browser and another for server. We can read .client. or .server. extension files.
  • For Typescript lovers ❤️ , this pull request will support ts | tsx | js | jsx files by default.
  • By using sui-jest browser will read files like:
    • helloWorldUseCase.test.js
    • helloWorldUseCase.browser.test.js
    • helloWorldUseCase.client.test.js
    • helloWorldUseCase.test.ts
    • component.test.tsx
  • By using sui-jest server will read files like:
    • helloWorldUseCase.test.js
    • helloWorldUseCase.server.test.js
    • helloWorldUseCase.test.ts

⚠️ This is a Breaking Change, because currently we are only wrapping jest doing nothing. And with this proposal, the cli api has changed.

Pending / TODO

  • Tested on a sui-domain playground
  • Avoid read tsx | jsx files on sui-jest server script
  • Pass not controlled parameters to jest script. Actually, if you pass a valid jest cli param, it will not be detected.
  • Let read jest.config.ts file.
  • Let to pass custom browser and server file config.
  • Test it with components.
  • Test it on production marketplaces.
  • Update Readme.md when we are agree.

📚 I've learned a lot doing this, do not hesitate to destroy my idea or purpose improvements! ❤️

Example

sui-jest browser // This will read file like index.test.js or index.browser.test.js for example
sui-jest server // This will read file like index.test.js or index.server.test.js for example

sui-jest browser --watch // Re-run tests when a file is changed
sui-jest server --watch // Re-run tests when a file is changed

sui-jest browser --coverage // Show cli coverage at the end of the cli
sui-jest server --coverage // Show cli coverage at the end of the cli

sui-jest browser --pattern // Specify a custom pattern where you have the tests
sui-jest server --pattern '**/__tests__/**/?(*.)+(spec|test).[tj]s?(x)' // Specify a custom pattern where you have the tests


program
.option('-W, --watch', 'Run in watch mode')
.option('--ci', 'Run a Firefox headless for CI testing')
Copy link
Collaborator

Choose a reason for hiding this comment

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

There isn't a plural version for Firefox. This should be one of the following:

1 - Run a Firefox headless browser for CI testing
2 - Run Firefox in headless mode for CI 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.

Thanks, I should check/update this description in order this is wrong I guess. Thanks again for detect it!


program
.option('-W, --watch', 'Run in watch mode')
.option('--ci', 'Run a Firefox headless for CI testing')
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment for the Firefox pluralisation also applies here


// TODO: Decide which is the default common config
const jestConfig = {
// All imported modules in your tests should be mocked automatically
Copy link
Collaborator

@danilucaci danilucaci Nov 6, 2023

Choose a reason for hiding this comment

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

We should remove all the commented options if they are not used. We can always read the official docs for the available API.

config = {...config, ...projectJestConfig}
}

args.push(...['--config', JSON.stringify(config)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to create a temporary array for the spread operation. [].push(arg1, arg2, arg3) works with comma separated arguments.


args.push(...['--config', JSON.stringify(config)])

require('jest').run([...args])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use only require('jest').run(args)? There is no need for the temporary array that is used for the spread operation. The args array is also available only inside the scope of the function, therefore, it can't be modified globally.

Copy link
Collaborator

@danilucaci danilucaci left a comment

Choose a reason for hiding this comment

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

Nice work! 👏🏻 👏🏻 👏🏻

@@ -0,0 +1,29 @@
#!/usr/bin/env node
/* eslint-disable no-console */
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed ?

@andresz1
Copy link
Member

andresz1 commented Nov 6, 2023

Nice 👏🏻

@giodelabarrera
Copy link
Collaborator

Hello, excellent iteration. If you allow me, I would like to share a few more of the design context of sui-jest when I built it 😀.

IMO, projects don't have the same needs; an example is within a monorepo as the example you are sharing: components and domain. sui.jest was created with the intention of being able to find the closest jest.config.js file and use it as configuration.

/components
/domain
  jest.config.js
  package.json
/src
jest.config.js
package.json

In this way, the domain directory could have a different configuration as environment=node if it is required, while another one at the root with environment=jsdom, which is fake dom jsdom made with js.

Regarding differentiating .server.js or .browser.js, my opinion is that this approach is good with sui-test since this runs the web browser with browser due to Karma, while in the server side does not. In the case of jest, all are always executed on server side whether they have node or jsdom. It is correct since it is thought for unit-test and integration-test (testing functionality and integrations with low resources, but more similar to the browser). Thus, the following sui-jest CLI API example would not be the reality IMO:

sui-jest browser // This will read file like index.test.js or index.browser.test.js for example
sui-jest server // This will read file like index.test.js or index.server.test.js for example

If there is, for a test, the need or use case the difference of being executed on a node or jsdom environment, I think there are possible options for avoiding a BREAKING CHANGE, for example, Jest environment configuration or by annotations in your test file

And as the last point to give context, my intention with sui-jest was not to create a wrapper with the common SUI philosophy (decoupling infrastructure); for that, the infrastructure name in the package name itself sui-jest. That is to say, jest CLI provides us with many options Jest CLI Options; therefore, we could save this and future efforts if we use them. That was why I didn't use commander so as not to close these options but to benefit us from all the Jest CLI API.

I hope that with these thoughts, I can help you decide where we want to go, simply, both paths will be accepted for me 🚀

@oriolpuig
Copy link
Contributor Author

First of all, thanks for your top explanation @giodelabarrera ❤️ . Now, I've more clear why sui-jest was developed as is; and not using commander.

If I'm not wrong, the purpose of SUI is to wrap and standardise as much as possible through all projects that uses SUI; and empower them to add little customisations. For example:

  • sui-lint: unifies rules for all SUI projects
  • sui-test: unifies how you can test for all SUI projects, and you can add custom configurations trough config.sui-test section on the most closer package.json where you run the script.
  • ... and other tools uses the same approach.

I see your point to give each project the 100% of the configuration to work, but maybe it would be nice to standardise it from scratch by defining a base configuration. Or, if it's needed, have different configuration for jsdom and node environments. I appreciate ❤️ a lot your feedback.

I'll answer you by topic, in order to improve the current approach without rewriting as I did:

sui-jest was created with the intention of being able to find the closest jest.config.js file and use it as configuration.

That's great! We missed this important point when we use it on our project. We have created a main jest.config.js file for all tests.

So, ok, i'm agree with you. This challenge could be solved by creating a jest.config.js file on each package/ folder and run the script inside those folders. Because if you run the script at root folder; as far as I know, it will pick the jest configuration from root 😓 , and that's what is happening to us. We have learned a lot here. Maybe a good point could be to improve the documentation with more examples? 🤔

In the case of jest, all are always executed on server side whether they have node or jsdom

Yes, by default jest runs on node, ok. In our projects that we usually run the tests on browser (for components, page and domain testing), by following the previous topic of having the jest.config.js file on each packages/ folder; it should work fine. And, if you need to run it on --env=node, you might need to have another setupTests.js file, for example.

For domain testing, we have two scenarios:

  1. In case your Use Cases are isomorphic (this is ideal), you should be able to run the Use Case test on browser and server tests, and it should works fine. Server test will fail when you use Browser API and will enforce you to think about if it should run only on browser or you should fix that. Thanks to that, you will have only 1 single test to maintain, and we can avoid to use .server.js and .browser.js tests. 💪🏻
  2. In case your Use Case runs ONLY on browser or server, as you said, you can add some Jest annotations and Jest will skip or run the test depending on it. Ok, we still having a single test to maintain.

In resume, for those cases, we should:

  • Have 2 scripts sui-jest --env=jsdom and sui-jest --env=node to run tests on both environments.
  • If you want to run a test on an specific testEnvironment, you will use jest annotations to enforce it.
  • Have 2 jest.config.js files, one by browser and another for server, to setup custom customisation for each environment, for example, a custom setupTest.js file. We will pass it to the npm script with --config flag. In case we need it.

As you have seen, by answering you I've purposed some how we can use the current implementation of sui-jest, in order to not force a breaking change.

I'm going to play a little but with those things and see possible new approaches. But I would like sui-jest follows the SUI mindset by avoiding to have full jest configurations on each project, and try to put the most common configurations on this package. Or maybe, expose toe configuration to outside. I'm not sure.

Thanks again for your feedback.

@jordevo jordevo self-requested a review November 9, 2023 21:48
@jordevo
Copy link
Contributor

jordevo commented Nov 9, 2023

Great job and contribution, @oriolpuig ! 👏

I understand @giodelabarrera 's point to allow for each project to have it's own configuration and a good thing this iteration brings is that it still allows for that while it gives a default config that could work for most if not all our projects.

I think we could even have different configurations for different types of projects (ie domain, components...) by declaring so on the project's package.json itself, couldn't we? Same as we do with configuration for other tooling such as sui-bundler.

I may lack some context as I am not familiar to Jest and what problems it comes to solve that we couldn't solve before.

All that said... again, good stuff, @oriolpuig ! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants