Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

add ablity to test error scenarios in test harness #317

Closed
wants to merge 13 commits into from

Conversation

domgreen
Copy link
Contributor

Adding the ability to check for error scenarios in the test harness.

@tro3 - mind casting an eye over this, please.

Seems to have broken the TestIntegration/ensure/update/case1 test but think we knew that would happen, think @tro3 already has a PR out for the fix.

@domgreen
Copy link
Contributor Author

Ah nerds, that test failed the CI check ... should I wait till after the PR from @tro3 is merged and rebase my changes on that one?

@tro3
Copy link
Contributor

tro3 commented Mar 12, 2017

@domgreen, the PR I have in the queue is about being able to -update the testcase. I don't know what you mean about TestIntegration/ensure/update/case1?

@domgreen
Copy link
Contributor Author

@tro3 Sorry, I was trying to say that is the -update test is failing but think your PR will fix it 😄

@tro3
Copy link
Contributor

tro3 commented Mar 12, 2017

@domgreen - two different things. The "-update PR" gives the test suite a capability to update the testcase.json file instead of comparing against it, when the -update flag is set. (This already exists for the final files.) It has nothing to do with the ensure/update cases, despite the unfortunate name coincidence.

What I can't figure out is how your modifications could have affected that test. I get the same result on my machine - master passes, but your PR fails. I'll dig.

@tro3
Copy link
Contributor

tro3 commented Mar 12, 2017

Aha! Apparently, ensure/case1 was always throwing an error! It was originally written as two commands, init then ensure and one is throwing an error. When I comment out the break command you added, the test passes. More digging required, but this has clearly uncovered a problem with the existing test. (But it also should have failed with an "unexpected error" and did not, so there may be an issue in the implementation as well.)

Copy link
Contributor

@tro3 tro3 left a comment

Choose a reason for hiding this comment

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

ensure/case1 is failing because init is being called when manifest.json already exists. If you remove the initial init command, the test will pass.

tc.t.Errorf("expected error %s, got error %s", want, got)
}
} else if !wantExists && gotExists {
tc.t.Errorf("%s error raised where none was expected", got)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fatalf should be used on line 71. If an unexpected error occurs, the rest of the test is invalid, and should just end.

@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@domgreen
Copy link
Contributor Author

Closing and moving to #319 as I've obviously mess up with git ... remember to constantly rebase.

@domgreen domgreen closed this Mar 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants