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

Remove /wordpress from test/linting ignore paths #20270

Merged
merged 6 commits into from
Jan 3, 2021

Conversation

johnwatkins0
Copy link
Member

Description

This resolves #20255 by removing /wordpress where it appears among linting/testing ignore patterns, essentially undoing everything in https://github.com/WordPress/gutenberg/pull/17296/files.

Having /wordpress/ in the ignore patterns was preventing Jest tests from being recognized when the project has /wordpress/ somewhere in the system directory path. According to @talldan
in #20255 (comment), the original reason for ignoring the directory no longer applies, so it can be removed.

How has this been tested?

Ran linting and tests locally after the change. Everything worked.

Screenshots

Types of changes

Change to test and linting configurations.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this @johnwatkins0.

Perhaps the wordpress folder should also be removed from the .gitignore file now that it's not expected to be there. What do you think?

@talldan talldan added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Feb 18, 2020
@johnwatkins0
Copy link
Member Author

Thanks, @talldan. I agree -- if there's no longer anything in plugin development process that creates a /wordpress directory, it doesn't need to be in .gitignore. I've updated.

@gziolo
Copy link
Member

gziolo commented Feb 18, 2020

Thank you for working on it. We should deprecate wp-scripts env first, before we land this PR. It is still referenced in some places in the root package.json of Gutenberg . In addition, if someone uses @wordpress/scripts and they run env script, they will see unexpected errors.

@talldan
Copy link
Contributor

talldan commented Feb 18, 2020

We should deprecate wp-scripts env first

@gziolo Ah, does that still install wordpress into a local folder? I didn't realise that.

It looks like the older env is still used for travis, so that'd be one of the first places to replace with the newer wp-env:

gutenberg/.travis.yml

Lines 83 to 99 in c76137b

npm run env:start
sleep 10
npm run env:install
cd ..
# Connect Gutenberg to WordPress.
npm run env connect
npm run env cli plugin activate gutenberg
fi
- |
if [[ "$INSTALL_COMPOSER" = "true" ]]; then
npm run env docker-run -- php composer install
fi
- |
if [[ "$E2E_ROLE" = "author" ]]; then
npm run env cli -- user create author [email protected] --role=author --user_pass=authpass
npm run env cli -- post update 1 --post_author=2

@gziolo
Copy link
Member

gziolo commented Feb 18, 2020

@gziolo Ah, does that still install wordpress into a local folder? I didn't realise that.

It probably does. At least, it's what happens locally :)

@gziolo
Copy link
Member

gziolo commented Dec 19, 2020

@johnwatkins0, we can land this changes now tfst @wordpress/env is widely used. Would you mind bringing this PR up to date?

There is also a note about ignored wordpress folder in README file of @wordpress/scripts package.

@johnwatkins0 johnwatkins0 force-pushed the fix/wordpress-in-ignore-paths branch from c674bc5 to a4c9327 Compare December 29, 2020 18:29
@johnwatkins0
Copy link
Member Author

There is also a note about ignored wordpress folder in README file of @wordpress/scripts package.

@gziolo This PR has been updated, and the README file was modified in 0fdb93a

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@gziolo gziolo added this to the @wordpress/scripts v13 milestone Jan 1, 2021
@gziolo gziolo merged commit 9dd9d28 into WordPress:master Jan 3, 2021
gziolo added a commit that referenced this pull request Jan 3, 2021
@gziolo gziolo removed this from the @wordpress/scripts v13 milestone Jan 4, 2021
@gziolo gziolo added this to the Gutenberg 9.8 milestone Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest tests don't run when /wordpress/ is somewhere in project path
3 participants