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

Fix most WebGL tests #7210

Merged
merged 13 commits into from
Sep 7, 2024
Merged

Fix most WebGL tests #7210

merged 13 commits into from
Sep 7, 2024

Conversation

davepagurek
Copy link
Contributor

  • Adds some getters to colors for now to get backwards compatibility
  • Fixes some test changes between the old runner and the current one
  • Fixes some actual bugs, not sure how those tests were ever passing:
    • A roll() test was actually testing tilt()
    • A buildGeometry test was ignoring the surrounding context's fill color and comparing that against white and things still worked
  • Turns off a thing that saves screenshots on test failures since they're all empty anyway

To do:

  • Fix RendererGL tests, something's off with clipping + blending
  • Fix all tests that load images (@limzykenneth do you know if fetch() of relative paths is supported in vitest?)

Also, some thoughts on screenshots: since it seems like vitest is able to save screenshots on test failures, I was reading this page https://vitest.dev/guide/snapshot#image-snapshots and they seem to have something like what we want from visual tests. Maybe this is worth using rather than the more custom saving/comparing code I had before?

@davepagurek davepagurek marked this pull request as draft September 4, 2024 00:23
@limzykenneth
Copy link
Member

fetch() on relative paths probably will work as it is running a dev server, whether we need to specifically set static asset paths or not I still have not look into it.

Turns off a thing that saves screenshots on test failures since they're all empty anyway

That is the failure screenshot that are automatically generated but the image snapshot solution may work as well, you can have a test.

There are some problems with having the test run properly in CI still and I may switch the provider from webdriverio to playwright but playwright does not support Chrome from vitest (uses Chromium instead) and the browser need to be installed with a separate command. Locally things seem to run fine so just need to solve CI.

@limzykenneth
Copy link
Member

Ok I think I may have solve things on my end. Not sure if it made sense for you to merge this first then I'll rebase and merge mine or do it the other way round.

@sproutleaf On a somewhat related note, when you are working on your code in the src/ folder, can you make sure that the commit hook's eslint task passes before merging to the dev-2.0 branch. That will help other contributors working on that branch to be able to commit as usual, I'll check future PR as well. @davepagurek I'll fix the lint task as well so at least even if the unit test CI task doesn't pass, the lint CI task should pass before we merge.

@davepagurek
Copy link
Contributor Author

Sounds good, feel free to merge yours and I can rebase this one later as there's still some more debugging I have left to do on it anyway. Also maybe see if #7215 looks good to you and we can get that one in too.

@limzykenneth
Copy link
Member

@davepagurek I'm in the process of marking tests that still need implementation to pass as todo so we get a clean pass of the test suite. Do you want me to mark the webgl ones as well (which you may need to resolve conflict with this PR) or you'd like me to leave them as is?

@davepagurek
Copy link
Contributor Author

I'm going to try to get the rest of these fixed tonight so maybe we can leave them as is for now, and I can mark any remaining WebGL ones?

@limzykenneth
Copy link
Member

Yeah, I'll leave those. The FES ones @sproutleaf added isn't working for me as well so probably need some fixing too. I'll merge where it's at now.

@davepagurek
Copy link
Contributor Author

Fixing some more tests, some other little bug fixes I encountered:

  • there was a bug where it would set alpha to 1 if the alpha param is falsy in a p5.Color, which was overriding alpha=0
  • I updated the loadImage function to return a promise that resolves when the image is fully loaded instead of a promise that resolves before it's done

@davepagurek
Copy link
Contributor Author

Also, for tests that load images, I added publicDir to the vite config to be the root of the test directory, and then tests are able to fetch stuff that uses an absolute path from there.

@davepagurek
Copy link
Contributor Author

Tests are all passing from earlier, just dealing with the test failures after merging dev-2.0 now!

@limzykenneth I see that _pixelsState is mostly turned into pInst now. I think p5.Image was doing something differently with that, setting this._pixelsState = this in its constructor. Seems like before _pixelsState is the thing that holds its pixels array, pixel density, etc, which for most stuff is the pInst, but for an image, would just be itself. I think it might make sense to keep it?

@davepagurek
Copy link
Contributor Author

whew were back, webgl tests pass!

Screenshot 2024-09-04 at 7 54 48 PM

now I have not yet checked that I haven't broken some non-webgl tests yet haha, that'll have to wait for another evening

@limzykenneth
Copy link
Member

The hierachical relationship between p5.Element, p5.Renderer, p5.Graphics, p5.Image, the p5 instance/canvas etc are things we need to sort out as it is a bit too complex now. Will need to have a chat about this I think.

@davepagurek
Copy link
Contributor Author

@limzykenneth sounds good, let me know if you want to call for that at some point!

For this PR, I think these are the remaining failing tests:
image

I'm going to address visual tests separately, so I think this one is ready to go

@davepagurek davepagurek marked this pull request as ready for review September 7, 2024 14:02
@davepagurek
Copy link
Contributor Author

ok actually I got #7251 working so I'm just going to merge everything in

@davepagurek davepagurek merged commit 3a850c6 into dev-2.0 Sep 7, 2024
1 of 2 checks passed
@davepagurek davepagurek deleted the fix/webgl branch September 7, 2024 18:01
@limzykenneth limzykenneth linked an issue Sep 12, 2024 that may be closed by this pull request
21 tasks
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.

[p5.js 2.0 RFC Proposal]: Build and test system update
2 participants