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

Added .err tests to close issue #479 #481

Merged
merged 1 commit into from
Jun 15, 2016

Conversation

TobiasWrigstad
Copy link
Contributor

@TobiasWrigstad TobiasWrigstad commented Jun 12, 2016

This closes issue #479 by adding support for tests that pass the compiler but fail at run-time. Tests with a .err file have their stderr redirected to stdout and thus we support also matching against stderr output, which is the case with #204. If tests that have an .err don't fail at run-time, the test is considered failed.

@TobiasWrigstad
Copy link
Contributor Author

@kaeluka I modified the test script to use a tmp file to flag whether a test was successful or not. This tmp file is deleted before each .err test is run and at the end of all tests to remove clutter.

If you think there is a better way, please make it so.

return
fi

rm -f /tmp/encore_test_failed
Copy link
Contributor

Choose a reason for hiding this comment

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

The run function should just return the exit code of the test. Or, alternatively do something like

if (($? != 0); then
  echo "exited with error code"
fi

Then this file shouldn't be necessary.

@TobiasWrigstad
Copy link
Contributor Author

I am not sure I get it.

When we are running .err tests using the run function we want to make sure that exit code is != 0, but when we run them outside of .err, we don't want that.

@kaeluka
Copy link
Contributor

kaeluka commented Jun 13, 2016

When running an .err test, we could check that run returned false; when running a .out test, we could check that run returned true (or that " exited with error code" is not in the output).

@TobiasWrigstad
Copy link
Contributor Author

This was the I tried, but I could not get it to work. Possibly because of some subshelling. Come to think of it, maybe run should end with exit rather than return?

I think grepping for a line in the output is worse than the file marker. If you have a more concrete suggestion how to propagate the exit codes that would be very helpful!

@kaeluka
Copy link
Contributor

kaeluka commented Jun 13, 2016

What'd work (modulo me not knowing bash well enough) is to store $? into a variable after running the test in run: local RET=$?. Then the cleanup can be done, and the last line would be return ${RET} or ${RET} (they're equivalent, IMHO).

The text in the output is also not very nice, returning a proper return value like this is best, IMO.

@TobiasWrigstad
Copy link
Contributor Author

That's exactly what I did on my first try and could not get to work! :(

@TobiasWrigstad
Copy link
Contributor Author

TobiasWrigstad commented Jun 13, 2016

@kaeluka I'll take another crack at this tomorrow or whenever I can find some time. If I cannot get it done by tomorrow, I propose we accept this anyways because the file solution isn't that bad.

(modulo any other things that need fixing ofc)

@TobiasWrigstad
Copy link
Contributor Author

@kaeluka My $0.02: If you haven't gotten around to this yet, please merge anyway to get this out of the way. You can come back to this later for gold plating.

@kaeluka
Copy link
Contributor

kaeluka commented Jun 14, 2016

I'll look at this in a bit.

@kaeluka
Copy link
Contributor

kaeluka commented Jun 14, 2016

Now we're in a grey area when it comes to merging. I committed on top of your branch @TobiasWrigstad, so technically I can't merge it.
The fixes work for me. If you look at them and like them: you can merge your own PR. I have reviewed everything else you have written and think it should go in.

@kaeluka
Copy link
Contributor

kaeluka commented Jun 14, 2016

btw, for you, me and possibly others who want to hear some horrible bash trivia:

I learned that OUTPUT=$(some_command) stores the stdout of some_command in $OUTPUT, and the return value of some_command in $? as you and me would've expected.

HOWEVER, local OUTPUT=... is syntax sugar for OUTPUT=...; local OUTPUT. The latter call always (or at least here) succeeds. So what messed up the return values was that we're using local variables, as we should. The solution is to declare OUTPUT local before the assignment. Wow. What a mess!

@TobiasWrigstad
Copy link
Contributor Author

What crap! Well done. I'll look at this today.

@TobiasWrigstad
Copy link
Contributor Author

I've merged and tested and squashed. Who can review and merge this now?

@kaeluka kaeluka merged commit f7c54c4 into parapluu:development Jun 15, 2016
@albertnetymk
Copy link
Contributor

I don't know if it's this PR that introduced err files, but negating err needs to be added to .gitignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants