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

create a grunt task to compare PDOM descriptions #138

Closed
zepumph opened this issue Jun 10, 2019 · 12 comments
Closed

create a grunt task to compare PDOM descriptions #138

zepumph opened this issue Jun 10, 2019 · 12 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jun 10, 2019

From a11y dev meeting today with @jessegreenberg, we were interested in prototyping around with a way to test description comparisons between two versions. We worked on a prototype today, and got to a commit point where we can compare the PDOMs on startup of working copy sim with the same from master (using git stashing). This is working, but there are some pretty big todos to think about before diving fully in. Let's talk more about this @jessegreenberg!

@zepumph
Copy link
Member Author

zepumph commented Jul 9, 2019

Notes from discussion:

usage:
cd gravity-force-lab
grunt pdom-comparison --sha=7584ghfjde758g905y4hbj

working copy vs 7584ghfjde758g905y4hbj

Algorithm:
get pdoms for current working copy
stash all repos
checkout 7584ghfjde758g905y4hbj
get pdoms for 7584ghfjde758g905y4hbj
restore copy

TODO how to get good PDOMs to compare (maybe compare PDOMs with deterministic fuzzing 
(multiple copies of the PDOM))
TODO how to display the results?
TODO: instead of just comparing working copy, be able to set the sha to compare with also

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jul 9, 2019

The notes in #138 (comment) illustrate that we discussed comparing PDOM from working copy against a given date of project by SHA. This way, we aren't locked into a single PDOM defined at publication time and we don't compare against master which would make it easy to miss things.

@zepumph
Copy link
Member Author

zepumph commented Jul 10, 2019

@twant and I worked on this today. We were able to get very far on testing the current working copy (even with unstashed changes) against a previous sim commit.

given this working copy change in GFLBScreenView:
image

And this command, where the sha is most recent commit in gravity-force-lab-basics:
grunt pdom-comparison --debug --sha=5ffa6afc09fb1f625d65e16a16f7d3c6e54255b4

We were able to see this output in git bash:
image

The next steps are marked as todos in the file, but here is what I think should be worked on and noted:

  1. this script needed to call CHIPPER/getPhetLibs, which is problematic because perennial should be dependencyless

  2. We are still only comparing startup PDOMs, and it may be nice to compare with some fuzzing.

  3. There was a fair bit of noise potential, and it is a bit hard to understand what the actual issues are, even from just a few days back. Here is an example from testing last week:
    image

  4. Use the load postMessage event for more efficient running of requirejs sims.

@zepumph
Copy link
Member Author

zepumph commented Jul 10, 2019

This task was breaking a MR that @chrisklus was working on, because checkout-master-all was unable to load the chipper getPhetLibs script. To bypass it, I did the same workaround that we do to support lint-everything from perennial: requiring only within the grunt task. Sorry for the trouble! Tagging @jessegreenberg too.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 13, 2020

While working on phetsims/tasks#1017, I discovered that this apparently has introduced a dependency on chipper in perennial.

In perennial.PDOMComparison.js:

const getPhetLibs = require( '../../../chipper/js/grunt/getPhetLibs' );
...
  // TODO: perennial shouldn't depend on chipper, https://github.com/phetsims/perennial/issues/138
  const dependencies = getPhetLibs( repo );

And this dependency has existed since 7/29/19. That seems potentially very bad, and should be addressed immediately, no? Assigning to @zepumph and labeling for developer meeting.

And perhaps another question is why does PDOMComparsion.js live in perennial?

@zepumph
Copy link
Member Author

zepumph commented Mar 16, 2020

This issue is for an experimental grunt task that isn't currently used. There is a dependencies getter in perennial that I was able to use instead of calling over to chipper. Thanks for the poke on the TODO.

@zepumph
Copy link
Member Author

zepumph commented Mar 16, 2020

Keeping labelled for dev meeting in case @pixelzoom had other thoughts beyond fixing the issue in master.

zepumph added a commit that referenced this issue Mar 16, 2020
@zepumph zepumph changed the title Great a grunt task to compare PDOM descriptions create a grunt task to compare PDOM descriptions Mar 26, 2020
@zepumph
Copy link
Member Author

zepumph commented Mar 26, 2020

The dependency on chipper was removed above, after discussing in dev meeting, nothing else is needed to be done here for that problem.

@zepumph
Copy link
Member Author

zepumph commented Aug 3, 2021

Over in friction, I'm about to tear up some PDOM descriptions to help support voicing. It would be just excellent if we could get something here working to support that effort. I would want to know if there were any problems I was creating earlier rather than later.

I think that my first idea above was a good one, but I didn't really know about the snapshot comparison tool, which is already pretty well set up to make comparisons with pixels. Perhaps I could add a module to it to also compare the PDOM HTML and perhaps some sort of serialization of alerts too (description and voicing while we're at it).

@zepumph
Copy link
Member Author

zepumph commented Aug 3, 2021

This issue was first about a grunt task. I'd rather create a new issue for experimenting with snapshot comparison, so I'll pick this up in phetsims/aqua#127. If things go well. I'll delete the grunt task and close this issue.

@zepumph
Copy link
Member Author

zepumph commented Aug 11, 2021

phetsims/aqua#127 is totally the way of the future. After giving @jessegreenberg a tour, I am ready to give up on this old strategy.

zepumph added a commit that referenced this issue Aug 11, 2021
@zepumph
Copy link
Member Author

zepumph commented Aug 11, 2021

Removed above. Closing

@zepumph zepumph closed this as completed Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants