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

Most of the unit tests are useless or fragile #300

Closed
mramato opened this issue Jul 7, 2017 · 2 comments
Closed

Most of the unit tests are useless or fragile #300

mramato opened this issue Jul 7, 2017 · 2 comments

Comments

@mramato
Copy link
Contributor

mramato commented Jul 7, 2017

There are a ton of tests that have a hardcoded glTF object, which itself isn't bad, but these tests also try and mock the pipeline extras added to loaded models. At best, this mocking causes the tests to be fragile because what constitutes the extras object can change int he future and these tests won't be updated (this has already happen). In many cases, the mocks are now incorrect and the tests aren't actually testing the right thing. For example, there are a lot of buffers being used for extras._pipeline.source which is actually a string (what was the cause of #297) but other files, such as compressTextureCoordinatesSpec.js are making the same mistake.

Hardcoding small valid glTFs is fine in most cases, but tests should never mock the pipeline extras and should instead call addPipelineExtras. (In general we should avoid mocks unless they are really needed).

@lilleyse I'm not sure who we have available, but it would be nice for someone to work on this soon. Both me and @tfili have ran into bugs every time we use gltf-pipeline. I think fixing our tests might help track a bunch of them down or at least stop things from getting worse.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 7, 2017

If this can be done in less than a day, let's do it soon but post glTF 2.0; otherwise, we need to wait until we have a bigger gltf-pipeline effort (perhaps not that far off) or a community contributor.

@ggetz ggetz added the cleanup label Jun 25, 2018
@lilleyse
Copy link
Contributor

The tests no longer hardcode internal stuff like pipeline extras.

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

4 participants