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

E2E Tests: Fix theme colors and font sizes. #18761

Merged
merged 3 commits into from Dec 4, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 26, 2019

Description

This PR takes an alternative approach to #18699, in improving the developer experience of E2E tests.

Instead of allowing tests to specify a required active theme, this PR seeks to normalize themes so that tests are mostly theme agnostic.

The first logical step for this was fixing editor colors and font sizes.

How has this been tested?

It was verified that tests that were failing in twentynineteen now pass.

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. .
@epiqueras epiqueras added this to the Future milestone Nov 26, 2019
@epiqueras epiqueras requested review from ajitbohra, nerrad, ntwb and talldan as code owners Nov 26, 2019
@epiqueras epiqueras self-assigned this Nov 26, 2019
@epiqueras epiqueras mentioned this pull request Nov 26, 2019
6 of 6 tasks complete
@epiqueras epiqueras requested review from gziolo and mcsf Nov 26, 2019
@gziolo gziolo requested review from youknowriad and aduth Nov 27, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 3, 2019

normalize themes so that tests are mostly theme agnostic.

Couldn't we achieve this by just nullifying the theme-specific customizations directly, so it falls back to the default editor appearance, rather than adding an additional layer of styling to the same effect?

In other words, could the normalize_theme_init function be instead changed to:

function normalize_theme_init() {
	remove_theme_support( 'editor-color-palette' );
	remove_theme_support( 'editor-font-sizes' );
}
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

I didn't know we could do that. Yes, that is much cleaner. Updated 😄

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

So should we now update the editor default colors and font sizes to match twentytwenty's or should we update the tests to expect the defaults?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 4, 2019

@epiqueras IMO, the latter. The tests should target the default appearance of the editor, not specific to any theme. And I don't know that there's a desire for the default appearance to align more closely to what's themed by TwentyTwenty.

@epiqueras epiqueras force-pushed the try/making-tests-theme-agnostic branch from f561445 to e5f9309 Dec 4, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 4, 2019

Yes, that makes a lot of sense. Updated 👍

@aduth
aduth approved these changes Dec 4, 2019
Copy link
Member

aduth left a comment

Well, the build error is disappointing, since it was what #18773 was intended to resolve.

But otherwise, this looks in good shape.

@epiqueras epiqueras merged commit da54d11 into master Dec 4, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Errored
Details
@epiqueras epiqueras deleted the try/making-tests-theme-agnostic branch Dec 4, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
* E2E Tests: Fix theme colors and font sizes.

* Normalize Theme: Remove theme supports instead of overwriting them.

* E2E Tests: Update to expect editor defaults for colors and font sizes.
@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Dec 12, 2019

This PR disables theme color palettes in the testing docker environment bundled with Gutenberg. Is that expected?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 12, 2019

Right! this seems not great for development @nosolosw We should find a way to enable plugins in tests but not in dev. That's the reason I didn't add the "disableAnimations" plugin to the mu folder, so you enable/disable in tests and in development.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 12, 2019

This reminds me of prior conversations at #14245 . If you're using the test container, shouldn't you expect that it behaves just as it would in tests? Why would we need a setup procedure for the tests-specific requirements to be put in place in the tests container (vs. those being there by default)?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 12, 2019

Yes, same discussion, for me the test container and the dev container are the same, I don't see why I need two environments to work locally.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 12, 2019

But at least as it stands today, they are two environments? And the changes here impact only the test environment?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 12, 2019

But at least as it stands today, they are two environments?

That's a good question. I don't know the answer since I use my own setup and the built-in setup changed so much lately.

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Dec 12, 2019

OK, so this was expected. So I've got a couple of more questions:

  • Are there other things that I should be aware of (in terms of not behaving like a vanilla WordPress)?
  • What would be the recommended way to develop Gutenberg in a way that the env works the same as a vanilla WordPress so developers can test things across themes/plugins/etc?

My preference would be to keep using the bundled environment (npm run env install), which is something that we've been promoting. I'm struggling to see the usefulness of a bundled environment that doesn't behave like a vanilla WordPress, though.

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Dec 12, 2019

But at least as it stands today, they are two environments? And the changes here impact only the test environment?

To clarify. I'm not sure what this means or what the difference between the test or dev container is. What I reported is that the bundled environment I use for development (I guess this is what you call dev container?) is affected by these changes (so I go to localhost:8889 and it doesn't work like a vanilla WordPress but has removed theme support for colors and font sizes).

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 12, 2019

But at least as it stands today, they are two environments? And the changes here impact only the test environment?

To clarify. I'm not sure what this means or what the difference between the test or dev container is. What I reported is that the bundled environment I use for development (I guess this is what you call dev container?) is affected by these changes (so I go to localhost:8889 and it doesn't work like a vanilla WordPress but has removed theme support for colors and font sizes).

This would be unexpected to me, yes. I think the default development as you describe should not be impacted by these changes. I'm not sure yet if that's a specific issue in the default development environment, or something we can hope to reconcile by consolidating between this and the "other" development environment.

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Dec 12, 2019

For what is worth, this is reproducible via (the instructions we give to contributors):

  • make a fresh clone of the Gutenberg repo
  • npm install && npm run build
  • npm run env install && npm run env start
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 12, 2019

What I reported is that the bundled environment I use for development (I guess this is what you call dev container?) is affected by these changes (so I go to localhost:8889 and it doesn't work like a vanilla WordPress but has removed theme support for colors and font sizes).

8889 is the tests container. 8888 is unaffected by these changes.

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Dec 12, 2019

8889 is the tests container. 8888 is unaffected by these changes.

mmm, is it? The documentation and the messages after npm run env install both mention 8889 as the port you use for accessing the dev environment. I can only access WordPress through 8889, trying 8888 doesn't work at all.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 12, 2019

Oh, you're using the Core env which only has one container.

I recommend you use: https://github.com/WordPress/gutenberg/tree/master/packages/env.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 12, 2019

If our current setup instructions will cause this plugin to be loaded as must-use, I think we either should change this to be a regular plugins, change the default environment to avoid loading mu-plugins, or update our getting started instructions to the recommended environment.

I think the first of these options would be the easiest for the short-term, to change this from being an mu-plugins to a regular plugins, then enabling it during the test setup.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 12, 2019

Why not,

update our getting started instructions to the recommended environment.

? We already send people there every time someone has an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.