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

Add colorsEqualEpsilon to ModelSpec test #7078

Merged
merged 3 commits into from
Sep 27, 2018
Merged

Add colorsEqualEpsilon to ModelSpec test #7078

merged 3 commits into from
Sep 27, 2018

Conversation

OmarShehata
Copy link
Contributor

This fixes the test failure in #7073.

I figured it would be useful to be able to test if colors are close enough so our tests are more flexible. Feedback is welcome!

I don't think the color change is due to #6877 because it looks like the scene in the spec doesn't have a globe initialized, so I assume that means it's not rendering the atmosphere. The box itself still looks fine in CesiumJS.

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Sep 26, 2018

@OmarShehata you should be able to do expect(color).toEqualEpsilon(value, epsilon)

@hpinkos
Copy link
Contributor

hpinkos commented Sep 26, 2018

We have a custom jasmine matcher that should automatically call Color.equalsEpsilon when you do expect().toEqualEpsilon

@OmarShehata
Copy link
Contributor Author

Ah, didn't realize that already existed. Thanks for pointing that out @hpinkos ! This seems to work.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 26, 2018

Thanks @OmarShehata! The tests pass locally now. There was a weird test failure in the travis build though. I just restarted it to make sure it wasn't related to this change. I'll merge this if that passes

@hpinkos
Copy link
Contributor

hpinkos commented Sep 27, 2018

@OmarShehata I'm not sure exactly why, but the change you made is causing all of the model tests to fail in travis. I'm guessing something isn't being cleaned up correctly, so after your tests runs it's causing scene.frameState.commandList to have an extra command.

You can reproduce this locally by running npm run test -- --browsers FirefoxHeadless --webgl-stub --failTaskOnError --suppressPassed on the command line


expect(scene).toRenderAndCall(function(rgba) {
expect(rgba).toEqualEpsilon([169, 3, 3, 255], 5); // Red
primitives.remove(m);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this needs to be moved outside the function block?

@OmarShehata
Copy link
Contributor Author

I think you were right @hpinkos !

@hpinkos
Copy link
Contributor

hpinkos commented Sep 27, 2018

Works great @OmarShehata, thanks!

@hpinkos hpinkos merged commit 3a57681 into CesiumGS:master Sep 27, 2018
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.

3 participants