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

temporarily disable failing unit test #2866

Closed
wants to merge 4 commits into from
Closed

temporarily disable failing unit test #2866

wants to merge 4 commits into from

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented Jun 26, 2020

The test was passing 10 days ago, but now it fails. The failure affects #2864, #2867 and possibly other in-flight PRs.

My plan is:

  1. Merge this PR and disable the test
  2. Investigate Investigate and fix Keras problem under Python 3.8 #2865 when I have time (or when someone else steps up to do it)
  3. Re-enable the test

@mpenkov mpenkov requested review from gojomo and piskvorky June 26, 2020 01:54
@mpenkov
Copy link
Collaborator Author

mpenkov commented Jun 26, 2020

@gojomo continued from #2864:

But why would we want to "know at a glance that everything still works" when in fact, everything still doesn't work – a test is broken.

In this case, "everything" really means "everything, except for stuff that we know is broken". Remember, at any one time gensim literally has dozens of bugs. Can you imagine what life for the maintainer would be like if each of those bugs caused a test to fail on each build?

Special-casing a test to be off for one Python version (if in fact that's even the real pattern-of-failure here – unclear from a single observation) just creates a dishonest green-light. It's like removing the battery from a smoke-detector instead of investigating "what's that burning smell?"

No, that's a false analogy. Let me adjust it so it reflects reality a little better.

We're sitting in a room working on an important time-sensitive project. Time is short, because everybody has a life outside of gensim. Suddenly, one of the many smoke detectors in the room starts beeping (remember, we have multiple tests that cover that functionality, and only one of them fails: Py3.8 under Travis). Our options:

  1. Work through the beeping. This is analogous to working with failing CI builds.
  2. Stop work and thoroughly investigate the problem. Bring out the smoke detector manual. Call customer support, etc. This is analogous to solving the problem properly.
  3. Disable the smoke detector, schedule a proper investigation once the important stuff is out of the way.

(1) is a PITA. (2) violates our priorities. (3) is this PR.

Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

Looks OK but can you remind me – why are we using Travis? I thought we switched the CI to Azure?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Jun 26, 2020

Travis does our Linux builds. Azure is Windows.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Jun 27, 2020

Closing in favor of #2868

@mpenkov mpenkov closed this Jun 27, 2020
@mpenkov mpenkov deleted the disable-keras branch June 27, 2020 12:10
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.

2 participants