Skip to content

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.

3 participants