Skip to content

test target dependencies are incomplete #1086

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

Closed
qcolombet opened this issue May 30, 2018 · 6 comments
Closed

test target dependencies are incomplete #1086

qcolombet opened this issue May 30, 2018 · 6 comments

Comments

@qcolombet
Copy link
Contributor

One thing I noticed, the dependencies are not correct in the cmake. In effect, if you run ninja test without building glow first, it will fail because the tests don't depend on glow being built. This is problematic as we may update glow, forget to rebuild, and run the tests and think that everything is okay whereas we would run the old code.

@qcolombet
Copy link
Contributor Author

Had a quick look at this today.
This is actually a bug/feature in cmake with the add_test command (https://gitlab.kitware.com/cmake/cmake/issues/8774).

There are two possible workarounds:

  1. Run all then test (basically do it manually)
  2. Add a custom target that depends on all the test executables and that invokes ctest

The problem with #2 is:
A. We have to manually maintain the list of executables. (Though with can hide that in some cmake function.)
B. We cannot use test for the target (e.g., we have to use check.)

Note: LLVM does exactly #2.

Let me know what you guys think and I'll either do a quick attempt at #2 or I'll close the issue.

@nadavrot
Copy link
Contributor

nadavrot commented Jun 4, 2018

I am okay with calling ninja check instead of ninja test.

@nadavrot
Copy link
Contributor

nadavrot commented Jun 4, 2018

CC @compnerd

@jfix71
Copy link
Contributor

jfix71 commented Jun 4, 2018

RE: 1. I always use alias ntest="ninja all && ninja test" in place of ninja test.

@qcolombet
Copy link
Contributor Author

In case people are interested into a check target that does exactly what @jfix71 said, I've given a stab at it and posted a PR:
#1104

@qcolombet
Copy link
Contributor Author

Fixed with commit 1116b5a.

Now, ninja check will do the re-build + test.

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

No branches or pull requests

3 participants