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

FSE: Fix JS unit tests #39326

Merged
merged 5 commits into from
Feb 11, 2020
Merged

FSE: Fix JS unit tests #39326

merged 5 commits into from
Feb 11, 2020

Conversation

noahtallen
Copy link
Contributor

@noahtallen noahtallen commented Feb 8, 2020

Note: this PR was split from #39000

This PR makes JS unit tests more consistent (in preparation for also having e2e and phpunit tests.) It also fixes several issues with the test suite.

Changes proposed in this Pull Request

  • Fixes an SPT test which broke after SPTs: Add new templates and template organization #39120. For some reason, it is not breaking on prod, but it is in this branch. That leads me to believe that the config in prod may not be testing everything correctly.
  • Renames unit tests to .test.js for easier detection (and to be consestent with .spec.js e2e tests)
  • Moves config files to bin/
  • Fixes running js unit tests locally.

JS unit and UI tests

The unit test command automatically runs test files that:

  1. Are in apps/full-site-editing
  2. Have the extension .test.js (or .ts, .tsx, jsx)

To run JS unit/component/ui tests:

npx lerna run test:js --scope='@automattic/full-site-editing' --stream

Testing instructions

  1. Test FSE build should pass
  2. From the wp-calypso root, run npx lerna run test:js --scope='@automattic/full-site-editing' --stream --no-prefix on the CLI and the tests should pass.

@noahtallen noahtallen added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Tests] Includes Tests [Goal] Full Site Editing labels Feb 8, 2020
@noahtallen noahtallen requested a review from a team February 8, 2020 02:53
@noahtallen noahtallen requested review from a team as code owners February 8, 2020 02:53
@noahtallen noahtallen self-assigned this Feb 8, 2020
@matticbot
Copy link
Contributor

@noahtallen
Copy link
Contributor Author

Screen Shot 2020-02-07 at 6 57 31 PM

You can see the passing tests in CircleCi by following the link

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is generally looking good 👍 Just left a couple of comments

You can see the passing tests in CircleCi by following the link

I guess this applies to some other PR? I didn't see these tests being run by CircleCI in this one.

apps/full-site-editing/bin/js-unit-config.js Outdated Show resolved Hide resolved
apps/full-site-editing/bin/js-unit-config.js Show resolved Hide resolved
@noahtallen
Copy link
Contributor Author

@tyxla Nice to see you again! :) You can see the tests in this test suite:

Screen Shot 2020-02-10 at 11 23 21 AM

noahtallen and others added 5 commits February 10, 2020 12:35
This way, everything can be consistent within the plugin
- Make it work better in the monoreo environment
- Move to bin folder
- Use test:* for test commands
- Move config to bin/folder
See https://github.com/Automattic/wp-calypso/pull/39120/files

In the above PR, the title was removed from the template selector.
Apparently, the tests were not correctly executing until now, so the
tests nedeed an update to stop referencing that title. Additionally,
one snapshot nedeed updated.

I guess the current test config in prod is not working correctly.
Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

Verified that the tests pass both locally and on CircleCI for this PR.

LGTM! :shipit:

@vindl vindl added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 11, 2020
@noahtallen noahtallen merged commit 0455ce2 into master Feb 11, 2020
@noahtallen noahtallen deleted the fix/fse-js-tests branch February 11, 2020 01:04
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.

4 participants