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

Automatic regression testing with CI #18574

Merged
merged 3 commits into from
Feb 22, 2020
Merged

Automatic regression testing with CI #18574

merged 3 commits into from
Feb 22, 2020

Conversation

munrocket
Copy link
Contributor

In Three.js repo we have several PR in a day and right now is really hard to formally validate correctness of the new PRs for maintainers. This idea was suggested by mrdoob and discussed here #16941, #13017. Also it makes possible to create a gallery with all existed examples in one page #17957.

This POC heavily optimized and provides test coverage in 98%. Details you can find here: https://github.com/munrocket/puppeteer-three. Before merge we probably need to create new contribution guides and add handy command in package.json that generate one screenshot.

@munrocket munrocket requested review from mrdoob and Mugen87 and removed request for mrdoob February 7, 2020 14:51
@mrdoob mrdoob added this to the r114 milestone Feb 7, 2020
@munrocket
Copy link
Contributor Author

munrocket commented Feb 10, 2020

2do list:

  • fix code style
  • migrate to serve from express and http-server
  • add id=startButton in webgl_performance_nodes button
  • rethink ci configs
  • fix indent style
  • add class .lbl in misc_animation_authoring
  • print diffs in console
  • update contribution guide
  • add files with extensions
  • color polyfill in travis console

@NicolasRannou
Copy link
Contributor

Very cool, out of curiosity, is it the same approach (determinist timers and so on) injection other projects such as the model viewer? https://modelviewer.dev/

@munrocket
Copy link
Contributor Author

munrocket commented Feb 10, 2020

@NicolasRannou probably not, but if they fix issue with offscreen-canvas or video tag we can take approach from there.

P.S. they doesn't have any randomness or timer hooks, I was inspired from gl-bench, this issue from Greggman and Miku-Miku from Takahirox.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
@munrocket
Copy link
Contributor Author

Example of error report in Travis terminal:

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 15, 2020

This PR has already 42 commits. Can you please force-push a single commit that contains all changes?

@munrocket
Copy link
Contributor Author

Yep.

@munrocket
Copy link
Contributor Author

I done different tests but after force push it gone:

  • Stress tests with 10 attempts, 2 from 10 fails.
  • Test to generate all screenshots from scratch.
  • Test that should fail and print result in Travis console.

This PR ready for merge. Also this similar settings just works in Travis under OS Windows:

Note that I am not changed contribution guide in Wiki page.

@munrocket
Copy link
Contributor Author

I fixed a problem with webgl_loader_svg witch is fails 2 times in previous stress test.
Updated results about robustness: 1/10 fail in Travis, 0/10 with CircleCI.

Under pressure Travis goes in infinite loop, because it can't resolve a promise.

@munrocket
Copy link
Contributor Author

@mrdoob actually this PR ready for merge, but need a feedback. Especially I am confused about folder naming diff, and probably I need to exclude test-diff from test command in package.json because different GPUs give different results.

Code is complicated and exists small bug that appeared in 1/10 of tests in Travis under Linux but in most of the cases it's work, and this is better than nothing I guess.

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2020

migrate to serve from express and http-server

Oh, I missed this decision. What's the reasoning?

@munrocket
Copy link
Contributor Author

Because concurrently with http-server become too noisy and I can't run it in silent mode https://github.com/kimmobrunfeldt/concurrently/issues/25 , so I need to use something like express from node script.

But having two similar package http-server and express in one project looks strange. serve is suitable for both cases: as a runner and from node.

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2020

Fine by me.

I personally do not use npm run dev anyway. I usually have a http-serve instance that I have installed globally.

I wonder if anyone uses npm run dev. As far as I remember it was added by @fernandojsg.

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2020

Merging this 🤞

@mrdoob mrdoob merged commit d2811b6 into mrdoob:dev Feb 22, 2020
@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2020

Many thanks!

@looeee
Copy link
Collaborator

looeee commented Feb 23, 2020

I wonder if anyone uses npm run dev.

I do. Well, I use the alias npm run start. I have that script on all projects I work on, and then I have a VSCode task that looks for and runs it automatically when I open a workspace.

@munrocket
Copy link
Contributor Author

Oh, classic... at least 1 message looks reasonable.

  1. we can allow fails in Travis with a Travis settings in .travis.yml
  2. we can ignore it for a while

Seems that I need to create a new PR, because this already merged?
Need to sync threejs dev branch from scratch anyway.

@munrocket
Copy link
Contributor Author

@mrdoob I am updated screenshots with this console command and all looks good in new PR #18708

npm run make-screenshot misc_controls_drag webgl_lightshafts webgl_materials_physical_reflectivity

At least one problem was with example renaming #18686

@munrocket
Copy link
Contributor Author

@looeee do you used 8080 port somehow in your settings? serve by default with 5000 port.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2020

Seems that I need to create a new PR, because this already merged?

Correct.

We've had some trouble with serve in the past and switched to http-server on purpose:, see #18424 (comment) and #14264. And yes, this problems occur now again. I suggest we move back to http-server and use serve only for the regression testing.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2020

Besides, is it possible to just log a warning if the screenshot for an example is missing? Right now, it seems the complete build fails which seems not appropriated for this use case.

@munrocket
Copy link
Contributor Author

munrocket commented Feb 23, 2020

@Mugen87 looks reasonable when you are prototyping, by the way we have exception list for Windows and Linux(CI) users.

Fixed http-server here #18711

@munrocket
Copy link
Contributor Author

We can make a new fake stage in Travis with name coverall for example and allow fail to it. It will check how all examples are covered with screenshots.

@looeee
Copy link
Collaborator

looeee commented Feb 23, 2020

@looeee do you used 8080 port somehow in your settings?

No, but I hadn't followed the recent switch to serve and port 5000. I'd prefer sticking with http-server unless people report issues with it.

I wonder if anyone uses npm run dev.

I do. Well, I use the alias npm run start.

By the way @mrdoob, it's not a big deal for me if this gets removed. However I would prefer if there's at least a watch script which runs rollup -c utils/build/rollup.config.js -w.

@takahirox
Copy link
Collaborator

takahirox commented Feb 24, 2020

Hi, after we switch to serve I face some troubles on Examples and Docs.

If I run npm run start and then access http://localhost:5000/examples or http://localhost:5000/examples/index.html, the examples don't appear because of ReferenceError: files is not defined. Somehow http://localhost:5000/examples/#webgl_animation_cloth works tho.

image

Similarly on http://localhost:5000/docs or http://localhost:5000/docs/index.html, the docs don't appear because of ReferenceError: list is not defined.

image

Can anyone here reproduce the same error? I use Windows 10 + Firefox/Chrome.

Update: Ah, sorry. I have missed the PR reverting to http-server. Never mind, thanks.

@munrocket
Copy link
Contributor Author

@takahirox hi! We reverted http-server, but it’s take some time to merge.

@mrdoob
Copy link
Owner

mrdoob commented Feb 25, 2020

Still one left:

ERROR! Diff wrong in 0.079 of pixels in file: css3d_youtube

https://travis-ci.org/mrdoob/three.js/builds/654693062

I guess we can ignore that one?

@munrocket
Copy link
Contributor Author

munrocket commented Feb 28, 2020

Added css3d_youtube in ingnore list. But problem with non-determinism in video tag still exist #18760. In less than 1/25 of builds webgl_kinect fails.

@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2020

Lets add webgl_kinect to the list then.

Also, I started to received emails every time the test pass. I used to only receive emails when test failed and when they get fixed. Any chance we can make it that way again?

@munrocket
Copy link
Contributor Author

munrocket commented Feb 28, 2020

@mrdoob looks strange, I am not changed anything with emails, do you started receive this emails yesterday after #18745?

Here documentation from Travis

notifications:
  email:
    recipients:
      - one@example.com
      - other@example.com
    on_success: never # default: change
    on_failure: always # default: always

emails can be hided.

@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2020

Yes, they started yesterday.

@munrocket
Copy link
Contributor Author

Fixed #18776

@munrocket
Copy link
Contributor Author

@Mugen87 because this stage exist in CI and usually we have ESLint in code editor. You have another cases?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 29, 2020

I was just surprised that the familiar command was suddenly gone, that's all.

@munrocket
Copy link
Contributor Author

Need help for good determinism in video/audio tag, other contributions also welcome.
In readme I am described current problems.

@mrdoob
Copy link
Owner

mrdoob commented Feb 29, 2020

I think we can ignore these examples. These tests are to make sure we don't break the renderer, video/audio is handled by the browser.

@munrocket
Copy link
Contributor Author

Sure that it can be solved with hooks, but #18782 for now.

@mrdoob
Copy link
Owner

mrdoob commented Feb 29, 2020

I just ran npm run make-screenshot physics_cannon_instancing and I got this... 🤔

13a0fc2?short_path=45d17f6#diff-45d17f64c08d97c9b0ec35e778f822df

@munrocket
Copy link
Contributor Author

Weird, did you ever try to run npm run test-e2e on your machine? I have a good screenshot.

@mrdoob
Copy link
Owner

mrdoob commented Feb 29, 2020

Weird, did you ever try to run npm run test-e2e on your machine?

I didn't. I'll try later.

@munrocket munrocket mentioned this pull request Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants