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.

Sign up for GitHub

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

Jump to bottom

Scripts: Remove npm run build from test-e2e default run #13420

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Scripts: Remove npm run build from test-e2e default run #13420

merged 1 commit into from
Jan 29, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 22, 2019

Description

This PR remove npm run build from pretest-e2e script which triggered build when running npm run test-e2e. This was introduced to ensure that files are built before running e2e tests. However, it is inconsistent with unit tests and causes issues when someone has npm run dev running in the background when developing code. Another issue is that this command was executed even when there were no changes introduced in the codebase since the last run.

@gziolo gziolo added [Type] Build Tooling Issues or PRs related to build tooling [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jan 22, 2019
@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 22, 2019
@gziolo gziolo self-assigned this Jan 22, 2019
youknowriad
youknowriad reviewed Jan 22, 2019
View reviewed changes
package.json
@@ -178,7 +178,7 @@
"publish:dev": "npm run build:packages && lerna publish --npm-tag next",
"publish:prod": "npm run build:packages && lerna publish",
"test": "npm run lint && npm run test-unit",
"pretest-e2e": "concurrently \"./bin/reset-e2e-tests.sh\" \"npm run build\"",
"pretest-e2e": "./bin/reset-e2e-tests.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this reset do?

Copy link
Contributor

Choose a reason for hiding this comment

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

After looking quickly, this also seems docker specific and it may break some workflows for people running the tests locally. I think we should move it similarly to the travis files.

Copy link
Member Author

@gziolo gziolo Jan 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not fully sure. This is what I see on the console:

gziolo$ ./bin/reset-e2e-tests.sh 
STATUS: Attempting to connect to WordPress...
STATUS: Resetting test database...
STATUS: Installing WordPress...
STATUS: Ensuring that files can be uploaded...
mode of '/var/www/html/wp-content/uploads' retained as 0767 (rwxrw-rwx)
STATUS: Current WordPress version: 5.0.3...
STATUS: Updating WordPress to the latest version...
STATUS: Checking the site's url...
STATUS: Activating Gutenberg...
STATUS: Installing a dummy favicon...

Copy link
Contributor

Choose a reason for hiding this comment

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

docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm -u 33 $CLI db reset --yes --quiet

It is definitely related to the docker setup.

Copy link
Member

Choose a reason for hiding this comment

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

It's not Travis-specific though; that script is used to reset the test environment so previous test runs don't pollute the results. If you don't reset the tests then when running things locally you might wind up in an unpredictable state.

Copy link
Contributor

Choose a reason for hiding this comment

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

It never happened to me and I never run the test but I agree with you that it's a possibility, the problem is that we can't offer an independent way of doing this reset no matter the WordPress setup used locally. Some use docker, others use vvv and others have local installs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that people run e2e tests against their production websites. It shouldn't be an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like Travis is super unhappy about removing reset script 🤷‍♂️

tofumatt
tofumatt reviewed Jan 22, 2019
View reviewed changes
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

What happens if you want to run the tests and the app hasn't been built? Can we at least check for that locally and warn the user about it?

@gziolo
Copy link
Member Author

gziolo commented Jan 24, 2019

What happens if you want to run the tests and the app hasn't been built? Can we at least check for that locally and warn the user about it?

Should we add check if build folder contains subfolders? Would it be enough?

@gziolo
Copy link
Member Author

gziolo commented Jan 29, 2019

Travis CI is happy now, I had to leave pretest-e2e to make sure that it works properly. This shell script seems to be very important.

@gziolo gziolo merged commit 6d500c2 into master Jan 29, 2019
@gziolo gziolo deleted the update/pretest-e2e branch January 29, 2019 11:12
@youknowriad
Copy link
Contributor

You should have left it but only in travis :)

@gziolo
Copy link
Member Author

gziolo commented Jan 29, 2019

You should have left it but only in travis :)

I'm looking forward to accepting PR you propose :)

@pento
Copy link
Member

pento commented Jan 29, 2019

GitHub needs a "wow" reaction. 😂😂😂😂😂

daniloercoli added a commit that referenced this pull request Jan 30, 2019
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
68b353c 
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (36 commits)
  Fixes plural messages POT generation. (#13577)
  Typo fix (#13595)
  REST API: Remove oEmbed proxy HTML filtering (#13575)
  Removed unnecessary className attribute. Fixes #11664 (#11831)
  Add changelog for RSS block (#13588)
  Components: Set type=button for TabPanel button elements. (#11944)
  Update util.js (#13582)
  Docs: Add accessbility specific page (#13169)
  Rnmobile/media methods refactor (#13554)
  chore(release): publish
  chore(release): publish
  Plugin: Deprecate gutenberg_get_script_polyfill (#13536)
  Block API: Parse entity only when valid character reference (#13512)
  RichText: List: fix indentation (#13563)
  Plugin: Deprecate window._wpLoadGutenbergEditor (#13547)
  Plugin: Avoid setting generic "Edit Post" title on load (#13552)
  Plugin: Populate demo content by default content filters (#13553)
  RichText: List: Fix getParentIndex (#13562)
  RichText: List: Fix outdent with children (#13559)
  Scripts: Remove npm run build from test-e2e default run (#13420)
  ...
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@gziolo @youknowriad
Scripts: Remove npm run build from test-e2e default run (#13420)
4482255
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@gziolo @youknowriad
Scripts: Remove npm run build from test-e2e default run (#13420)
0865c68
@aduth aduth mentioned this pull request Apr 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tofumatt tofumatt tofumatt left review comments

@youknowriad youknowriad youknowriad approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

Assignees

@gziolo gziolo

Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Milestone
5.0 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
4 participants
@gziolo @youknowriad @pento @tofumatt