Skip to content

Temporarily shifted to tests working on all machines #34

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

Merged
merged 1 commit into from
Aug 2, 2018
Merged

Conversation

anshuman23
Copy link
Owner

@josevalim to at least have tests on master that work for everybody without fail while the problems with the original are being diagnosed, I have condensed the functionality of the tests by grouping similar things together and by having only 5 different tests.

I tested these by running a shell script for around 50 times on a friend's machine and there is no failure.

This is only temporary and once the problem with the original tests are figured out, we will revert back to them.

@anshuman23 anshuman23 merged commit 7f00657 into master Aug 2, 2018
@josevalim
Copy link
Contributor

Sorry, I don't believe we should merge this, as it does not fix the problem, it only hides it. It is better to keep the tests failing until we come with the proper fix.

@anshuman23
Copy link
Owner Author

Sorry I merged it immediately @josevalim I was going to submit a PR for the new matrix functionalities and didn't want a conflict.

I can put the original tests back if you think so, but then I was thinking if people who use Tensorflex come across the tests failing they might think that the code doesn't work at all, while in fact the same stuff in the test code actually runs outside the tests in IEx.

Would it not be better to have the problematic expanded original tests in another branch while we figure out the fix? Because these condensed tests prove that the tests run fine when lumped together, so the functionality is not the problem, something else is.

What do you think? I'll put in the original tests back if you think that is better.

@josevalim
Copy link
Contributor

I can put the original tests back if you think so, but then I was thinking if people who use Tensorflex come across the tests failing they might think that the code doesn't work at all,

Before fixing the tests, we actually do not have a guarantee that the code works. Since the test fails from time to time, we know there are specific scenarios that will make the system fail, and at the moment we have not guaranteed that those scenarios cannot happen in production.

Hopefully more people looking at the tests means we can find the root cause sooner.

@anshuman23
Copy link
Owner Author

@josevalim I see your point. I have reverted the tests back to the way they were in the next PR #35 containing new matrix functions.

Sorry for not directly removing the commit made here and then adding the new PR.

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