Skip to content

Remove end to end tests #181

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

Closed
wants to merge 1 commit into from
Closed

Remove end to end tests #181

wants to merge 1 commit into from

Conversation

eulertour
Copy link
Member

The frame comparison tests make pixel-perfect comparisons which are too sensitive to test effectively. The tests are currently giving false negatives for every commit. This PR removes the tests until we can decide on a more effective way to test.

@eulertour
Copy link
Member Author

Pinging @huguesdevimeux for their input.

@huguesdevimeux
Copy link
Member

The tests work.
I just found why they were failing, I will make a PR soon. They undercovered two bugs : one related to the core of manim and one related to #98.

One again, I'm deeply convinced that removing tests when they fail is not the right solution and it goes against the very essence of testing.

@eulertour
Copy link
Member Author

It's not that we should remove all tests, only that we shouldn't have tests that fail when nothing is wrong.

The failures on each commit are confusing, and even with a fix it likely won't be sustainable to ensure each pixel of output is never changed. It'd probably be best to rethink our approach to testing.

@huguesdevimeux
Copy link
Member

huguesdevimeux commented Jul 8, 2020

It's not that we should remove all tests, only that we shouldn't have tests that fail when nothing is wrong.

Believe me, nothing was not wrong.

These tests were at the beginning supposed to be useful when adding a new graphical functionality, like a new animation, to be sure that the new feature has not broken anything. They are not designed IMO for other additions, as the CONFIG refactoring.
I do think those tests are useful, they allowed me to spot a lot of bugs. But If you have a better idea, that's cool.
And I agree, we should have other tests.

@leotrs
Copy link
Contributor

leotrs commented Jul 8, 2020

I think we should make all of these tests optional, not remove them, until such a time when they are passing again. I agree with @huguesdevimeux that the tests do work in principle. Also it seems like #185 is going to deprecate this PR?

@eulertour eulertour closed this Jul 10, 2020
@eulertour eulertour deleted the remove-broken-tests branch July 10, 2020 05:48
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