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

loadAppTest passes on ChromeHeadless #32522

Merged
merged 1 commit into from Jan 6, 2020
Merged

Conversation

islemaster
Copy link
Contributor

I've been trying to run tests in ChromeHeadless on my local machine, since PhantomJS isn't working properly on recent versions of Ubuntu. Eventually, I'd like us to use ChromeHeadless in our CI builds too, but to do that, we need to fix up all the tests that fail on this browser.

One of the tests that fails is loadAppTest project level share images uploads a share image for a non-droplet project (instead of writing the level). I believe this fixes that test under both ChromeHeadless and PhantomJS.

See also

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

const expectedPngSize = /PhantomJS/.test(window.navigator.userAgent)
? 523181 // PhantomJS
: 38961; // ChromeHeadless
expect(blob).to.have.property('size', expectedPngSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ChromeHeadless writes a much, much smaller PNG than Phantom does. Unfortunately, this is a tough one to make work both ways without weakening this assertion somehow, so I'm going with a little browser detection. I'm open to pushback here - if we think we don't really care about the size of the generated PNG, we could just remove this assertion, or check that the size is at least 1k.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you been able to visually inspect the PNG generated by ChromeHeadless to confirm that it's actually correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question. I haven't for this case, but I'm going through a similar exercise for a different test that's comparing dataURIs, and while the PNG comes out correct, the difference in size is astounding.

PhantomJS ChromeHeadless
523kB 40kB
expected expected-chrome

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Go, ChromeHeadless!

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

browser detection in this context is definitely a little funky, but it's a little funky that this test cares about image size in the first place so I'm not all that concerned.

@islemaster islemaster merged commit 9be6091 into staging Jan 6, 2020
@islemaster islemaster deleted the loadapptest-chromeheadless branch January 6, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants