Skip to content

unit test Makefiles: add missing source files #2877

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Sep 18, 2018
Merged

unit test Makefiles: add missing source files #2877

merged 11 commits into from
Sep 18, 2018

Conversation

tautschnig
Copy link
Collaborator

No description provided.

@tautschnig tautschnig mentioned this pull request Aug 31, 2018
Merged
@tautschnig tautschnig changed the title jbmc/unit/Makefile: add missing source files unit test Makefiles: add missing source files Aug 31, 2018
@tautschnig
Copy link
Collaborator Author

Title amended and similar fixes applied to unit/.

owen-mc-diffblue
owen-mc-diffblue approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@owen-mc-diffblue owen-mc-diffblue left a comment

Choose a reason for hiding this comment

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

Is there some way we can stop this from happening in future?

thk123
thk123 approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

I don't know if possible - but an extra check to check the unit tests from cmake report the same number of passes as make would catch this.

@owen-mc-diffblue
Copy link
Contributor

@tautschnig Does the commit " fixup! Visual Studio does not strip trailing zeros in hexfloat mode" only change formatting? If so, it should be renamed.

@tautschnig
Copy link
Collaborator Author

Does the commit " fixup! Visual Studio does not strip trailing zeros in hexfloat mode" only change formatting? If so, it should be renamed.

Sorry, I should have added the "work-in-progress" label earlier: the Windows tests are failing, and I'm adding commits to debug this. Will clean up the history once I've reached a state where all tests are passing.

I am also looking into (portable?) ways of sanity checking the number of tests being run.

@tautschnig
Copy link
Collaborator Author

@owen-jones-diffblue @thk123 Could you please take another look as I have added code to sanity-check the number of tests being run.

thk123
thk123 approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

All seems good - one idea but non-blocking, can always improve in future

@tautschnig tautschnig requested a review from kroening as a code owner September 3, 2018 11:19
@tautschnig tautschnig force-pushed the jbmc-unit-tests branch 3 times, most recently from 8c49bc2 to 640cf4c Compare September 3, 2018 11:48
@peterschrammel peterschrammel removed their assignment Sep 3, 2018
thk123
thk123 reviewed Sep 3, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Awesome! Bit confused about the exceptions though?

unit/Makefile Outdated
CATCH_TEST = unit_tests$(EXEEXT)
N_CATCH_TESTS = $(shell \
cat $$(find . -name "*.cpp" \
-and -not -name "miniBDD_new.cpp" \
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ How come we exclude this? This seems to be a legitimate catch test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, maybe we should. The relationship to miniBDD.cpp is a bit weird, though.

unit/Makefile Outdated
N_CATCH_TESTS = $(shell \
cat $$(find . -name "*.cpp" \
-and -not -name "miniBDD_new.cpp" \
-and -not -name "catch_example.cpp") | \
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this run though? Included in the Makefile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we maybe just remove that example by now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favour of removing it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - docs probably make more sense that a live example!

owen-mc-diffblue
owen-mc-diffblue approved these changes Sep 3, 2018
View reviewed changes
unit/Makefile Outdated
N_CATCH_TESTS = $(shell \
cat $$(find . -name "*.cpp" \
-and -not -name "miniBDD_new.cpp" \
-and -not -name "catch_example.cpp") | \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favour of removing it

@tautschnig
Copy link
Collaborator Author

I'm sorry, this has grown way more than I had expected it would. Please let me know if I should refactor this into multiple PRs. Reviewing commit-by-commit should still be reasonable.

thk123
thk123 approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

I'm happy but I'd probably punt the bdd test change into a separate PR just as I don't understand it well enough to review it.

@tautschnig tautschnig force-pushed the jbmc-unit-tests branch 3 times, most recently from a94b01a to 49328be Compare September 4, 2018 07:21
allredj
allredj reviewed Sep 18, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

Passed Diffblue compatibility checks (cbmc commit: bd55a28).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85124505

@tautschnig tautschnig merged commit 7de9df8 into diffblue:develop Sep 18, 2018
@tautschnig tautschnig deleted the jbmc-unit-tests branch September 18, 2018 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allredj allredj allredj left review comments

@thk123 thk123 thk123 approved these changes

@kroening kroening kroening approved these changes

@peterschrammel peterschrammel peterschrammel approved these changes

@owen-mc-diffblue owen-mc-diffblue owen-mc-diffblue approved these changes

@cesaro cesaro Awaiting requested review from cesaro

@cristina-david cristina-david Awaiting requested review from cristina-david

@mgudemann mgudemann Awaiting requested review from mgudemann

@NathanJPhillips NathanJPhillips Awaiting requested review from NathanJPhillips

@pkesseli pkesseli Awaiting requested review from pkesseli

@smowton smowton Awaiting requested review from smowton

@chrisr-diffblue chrisr-diffblue Awaiting requested review from chrisr-diffblue

Assignees

@kroening kroening

Labels
bugfix Build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tautschnig @owen-mc-diffblue @thk123 @kroening @peterschrammel @allredj