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

Fixed the ordering of images for regression testing #1307

Merged
merged 1 commit into from
May 11, 2015

Conversation

aashish24
Copy link
Contributor

So that the cdash will show them correctly in the right
order

@jbeezley @doutriaux1 @ThomasMaxwell please review

So that the cdash will show them correctly in the right
order
@doutriaux1
Copy link
Contributor

Thanks!

doutriaux1 added a commit that referenced this pull request May 11, 2015
Fixed the ordering of images for regression testing
@doutriaux1 doutriaux1 merged commit 66bab1b into release May 11, 2015
@jbeezley
Copy link
Contributor

That caused the error to go away completely when it should have just switched the images in cdash. This line makes me very suspicious. I think an IOError is raised in the try block somewhere causing the test to return successfully when it should actually fail. If there really is a reason to do this, the condition should probably be more narrowly defined.

@doutriaux1
Copy link
Contributor

what are you saying @jbeezley ? You don't like "magic"?
More seriously should we worry about this now, or open a new issue for 2.3? I'll leave it up to you.

@jbeezley
Copy link
Contributor

I would say leave it for 2.3 and go over all of the tests to make sure failures are reported correctly (as in #1015).

@aashish24 aashish24 deleted the fix_vcs3d_test_image_order branch May 12, 2015 13:22
@aashish24
Copy link
Contributor Author

@jbeezley agree with you. it should have ret = (anything but 0). I will push a branch post 2.2 release.

@aashish24 aashish24 mentioned this pull request May 22, 2015
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