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 the use of popular plugins in e2e tests #15940

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented May 31, 2019

This PR blocks #15770

These tests have been up for a long time now and didn't prove to be fruitful. Also, it's blocking us from adding warnings to the console as some plugins could trigger console messages.

While working on this, I also noticed that the e2e test jobs are splitted into 4 separate jobs per role. I assume this is an attempt to speedup the tests but I'd personally question whether it's achieving its purpose.

  • Travis don't always run the jobs in parallel (I know there are limits sometimes)
  • The big parts of these jobs is about setting up the environment which is common to all of them and splitting doesn't help here
  • It also obscures the jobs a little bit as someone unfamiliar the setup would wonder why there are so many jobs.

I don't think I will touch this in this PR but I'd love thoughts. A single job per user would be so much clearer.

Closes #13038.

@youknowriad youknowriad self-assigned this May 31, 2019
@youknowriad youknowriad added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label May 31, 2019
@swissspidy
Copy link
Member

Just noting here that merging this would close #13038.

@gziolo
Copy link
Member

gziolo commented Jun 3, 2019

Just noting here that merging this would close #13038.

Yep, it seems like this is not going to happen so we should close 🙁

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.

These tests have been up for a long time now and didn't prove to be fruitful. Also, it's blocking us from adding warnings to the console as some plugins could trigger console messages.

Yeah, those plugins were added to help catch regressions but it never detected any errors... In fact, we had this case where one of the selectors was removed by accident and our e2e tests didn’t help to catch it. Maybe we should add e2e tests which verify that all public APIs are present? It could be something that would be executed as part of plugin release - similar to how we measure performance.

@gziolo gziolo added this to the 5.9 (Gutenberg) milestone Jun 3, 2019
@youknowriad youknowriad merged commit 74b27d2 into master Jun 3, 2019
@youknowriad youknowriad deleted the try/remove-popular-plugins branch June 3, 2019 09:01
@aduth
Copy link
Member

aduth commented Jul 10, 2019

I don't think I will touch this in this PR but I'd love thoughts. A single job per user would be so much clearer.

I don't think we should make this change. There's a ton more context to be found at #15159 and #14289 on this point.

Travis don't always run the jobs in parallel (I know there are limits sometimes)

Per #14289 (comment), we can have 15 containers running in parallel for the organization. There's a limit, yes, but the faster each individual task runs, the faster they clear up for another task to use. We should leverage parallelization as much as we can, as long as it helps reduce the average build time in a relatively proportional manner.

The big parts of these jobs is about setting up the environment which is common to all of them and splitting doesn't help here

This is true, and there's some discussion in #15159 about how we can try to improve this by a combination of (a) reducing build time, (b) preconfiguring Docker images, (c) reusing assets between tasks.

It also obscures the jobs a little bit as someone unfamiliar the setup would wonder why there are so many jobs.

I don't know that anyone should really care about this. My expectation is they should go to the failing task and look at the logs. That there are many tasks with similar names doesn't make a difference in my view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests: Activate more popular plugins in the e2e test run on Travis
4 participants