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

export more system/compiler info #75

Merged
merged 1 commit into from
Sep 2, 2015
Merged

export more system/compiler info #75

merged 1 commit into from
Sep 2, 2015

Conversation

nschloe
Copy link
Contributor

@nschloe nschloe commented Jun 29, 2015

As requested by @gahansen.

@nschloe
Copy link
Contributor Author

nschloe commented Aug 28, 2015

I believe this is ready for merging. :)

@bartlettroscoe
Copy link
Member

It would be good if we could add some checks to the automated tests to make sure this information is being set correctly. It may be hard to write portable tests for the contents of these variables but we can check to make sure they are there. We need to move TriBITS closer to Self Sustaining Software and that means we need to be adding automated tests as we add new features. In can point out where to add checks for these in the test suite.

@nschloe
Copy link
Contributor Author

nschloe commented Aug 29, 2015

I can point out where to add checks for these in the test suite.

Yes, please.

@bartlettroscoe
Copy link
Member

Put in MESSAGE() commands to print these new variables in:

TriBITS/test/core/ExamplesUnitTests/DummyPackageClientCMakeLists.txt

and then put in grep statements for these variables in:

TriBITS/test/core/ExamplesUnitTests/CMakeLists.txt

(just grep for RunDummyPackageClientBulid.cmake and you will see the other regex expressions).

For these tests to be portable, you will likely not be able to inspect the contents of these variables, just that they are present in the export file. But at least that will stop someone from accidentally deleting these and the automated tests not noticing. Add these tests as a new commit, then revert the commit that added the variables to the XXXConfig.cmake file and confirm the tests fail!

Writing stronger tests that look at the contents of these variables would require us too set up a mock env with mock compilers so they are the same on every platform. There are other options too but the above "existence" test is the bare minimum test. I am okay with an existence tests for now.

In general, if you are adding new features or extending the code and never add new tests or extend existing tests, that is a big red flag. Don't you agree?

Make sense?

@bartlettroscoe
Copy link
Member

Wait, this is the project-level file, not the package-level file? I don't think the project-level file is under any automated testing in TriBITS. Also, I am not sure that a project-level export file is going to be used in the refactoring in #63 at all. So I guess since there is no testing for this file now and I their may not be a long-term future for this file (if #63 goes well), then I guess I can go ahead and merge into TriBITS, untested, since it is not going to break anything related to any of my projects.

@nschloe
Copy link
Contributor Author

nschloe commented Aug 30, 2015

Wait, this is the project-level file, not the package-level file?

Indeed. @gahansen would certainly be glad about a merge.

@bartlettroscoe bartlettroscoe merged commit 7bdbc9c into TriBITSPub:master Sep 2, 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.

2 participants