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

CMake based AppVeyor builds are no longer running tests #6052

Closed
mback2k opened this issue Oct 7, 2020 · 29 comments
Closed

CMake based AppVeyor builds are no longer running tests #6052

mback2k opened this issue Oct 7, 2020 · 29 comments

Comments

@mback2k
Copy link
Member

@mback2k mback2k commented Oct 7, 2020

I did this

I looked at the AppVeyor builds and saw the quite astonishing short build times for those builds with tests (approx. 10 minutes).

Then I saw this on many builds:

...
Warning: test3013 not present in tests/data/Makefile.inc
Warning: test3014 not present in tests/data/Makefile.inc
Warning: test3015 not present in tests/data/Makefile.inc
TESTFAIL: No tests were performed
TESTDONE: 1387 tests were considered during 116 seconds

Manually going back in time and bisecting the AppVeyor history shows that commit ad64169 seems to break the tests:

TESTDONE: 64 tests out of 64 reported OK: 100%
TESTDONE: 1299 tests were considered during 247 seconds.
TESTINFO: 1235 tests were skipped due to these restraints:

Unfortunately that commit points to the wrong PR and I cannot see if it was sufficiently tested before merging.

I expected the following

Tests to be running like they did before ad64169:

TESTDONE: 1033 tests out of 1033 reported OK: 100%
TESTDONE: 1317 tests were considered during 707 seconds.
TESTINFO: 284 tests were skipped due to these restraints:

cc @bagder @MarcelRaad @Lekensteyn

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 7, 2020

Lessons learned: maybe we can modify the CI to check for an expected minimum number of successful tests being run.

@bagder
Copy link
Member

@bagder bagder commented Oct 7, 2020

TESTFAIL: No tests were performed this in particular should at least be a reason for failure in the CI....

bagder added a commit that referenced this issue Oct 7, 2020
... and make TESTFAIL stand out a little better by adding newlines
before and after.

Reported-by: Marc Hörsken
Issue: #6052
@bagder
Copy link
Member

@bagder bagder commented Oct 8, 2020

#6053 seems to do its job for the appveyor builds...

@bagder
Copy link
Member

@bagder bagder commented Oct 8, 2020

@bagder
Copy link
Member

@bagder bagder commented Oct 8, 2020

When cmake is used on appveyor, make test is not invoked and thus the test servers and tools are not built!?

Was that perhaps done by the default cmake --build before?

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 8, 2020

Was that perhaps done by the default cmake --build before?

Seems like this commit c2ab249 before ad64169 is the root cause for this.

@bagder
Copy link
Member

@bagder bagder commented Oct 8, 2020

It seems cmake --build . -- testdeps after the build will build the stuff in tests/.

bagder added a commit that referenced this issue Oct 8, 2020
... for the builds were testing is enabled.

Fixes #6052
bagder added a commit that referenced this issue Oct 8, 2020
(even for builds that don't actually run the tests)

Fixes #6052
@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Oct 8, 2020

@bagder

When cmake is used on appveyor, make test is not invoked and thus the test servers and tools are not built!?

Was that perhaps done by the default cmake --build before?

That's what the -DBUILD_TESTING CMake argument is or was for.

@bagder
Copy link
Member

@bagder bagder commented Oct 8, 2020

Ah right. But that doesn't seem to work (anymore)...

bagder added a commit that referenced this issue Oct 9, 2020
Fixes #6052
@bagder bagder added the help wanted label Oct 9, 2020
@bagder
Copy link
Member

@bagder bagder commented Oct 9, 2020

I've tried numerous things now but I can't seem to fix this. I need help!

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 9, 2020

@Lekensteyn @MarcelRaad can you a take a look at this maybe? Also: would #6036 be an option to solve this?

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Oct 12, 2020

@Lekensteyn @MarcelRaad can you a take a look at this maybe?

I'm not familiar enough with CMake to help here, sorry.

@jeroen
Copy link
Contributor

@jeroen jeroen commented Oct 12, 2020

Perhaps appveyor updated their cmake? https://www.appveyor.com/updates/

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 12, 2020

I'm not 100% sure but suspect the following removal was the root cause.
So

option(BUILD_TESTING "Build tests" "${PERL_FOUND}")
if(NOT PERL_FOUND)
  message(STATUS "Perl not found, testing disabled.")
elseif(BUILD_TESTING)
  include(CTest)  ### will enable_testing()
  add_subdirectory(tests)
endif()
@snikulov
Copy link
Member

@snikulov snikulov commented Oct 12, 2020

After digging changes and logs, looks like it was an intentional change to avoid test target name clash.
It looks like the following command should build dependencies and then run tests cmake --build . --target test.
Will try to debug/fix it with AppVeyor on the local project clone.

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 12, 2020

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 13, 2020

@snikulov looks better, but the test flags seem to be missing and I don't get the point of your second commit. TFLAGS should work on Windows. For AppVeyor the tests should be build by CMake, but then run using the existing run bash.exe runtests.pl execution.

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 13, 2020

@mback2k yes, TFLAGS should work, but using ${...} is Unix specific. For Windows native shell it should be %...% AFAIK. I'm opposed bash usage, but you can try to run CMake from bash. Probably it will work.

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 13, 2020

And my assumption not working :( Adding %TFLAGS% to the command produce the error:

Building Custom Rule C:/projects/curl-ckq2k/tests/CMakeLists.txt
  Unknown option: %TFLAGS%
@snikulov
Copy link
Member

@snikulov snikulov commented Oct 13, 2020

@bagder @mback2k I'm not familiar with Perl, so could you please explain where env variable TFLAGS comes from?
Adding a custom command in CMake will be something like that
perl runtests.pl [options] env{TFLAGS}.
When and where TFLAGS is defined? Is it available during configuration? Is it possible to pass TFLAGS through file?

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 13, 2020

Okay, it seems like TFLAGS as an environment variable for make is not currently used in appveyor.yml, instead the following arguments are passed directly to runtests.pl: -a -b$(($(echo '%APPVEYOR_API_URL%' | cut -d'/' -f3 | cut -d':' -f2)+1)) -p !flaky %DISABLED_TESTS%.

You will at least need to find a way to pass the following via cmake to runtests.pl: -a -p !flaky %DISABLED_TESTS%. The parameter -b is not important anymore as the tests use dynamic ports anyway.

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 13, 2020

yeah... still digging :) Added - cmd: set TFLAGS=%DISABLED_TESTS% to appveyor.yml try to figure out will it helps

snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests
Changed placeholder name from ${TFLAGS} to just TFLAGS
  to avoid platform shell issues

Closes curl#6052
snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests

Closes curl#6052
snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests

Closes curl#6052
snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests

Closes curl#6052
snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests
Removed curly braces (${TFLAGS} -> $TFLAGS)

Closes curl#6052
@snikulov
Copy link
Member

@snikulov snikulov commented Oct 13, 2020

@mback2k Could you please check? https://ci.appveyor.com/project/snikulov/curl-ckq2k
Looks like it works now.
I removed curly from ${TFLAGS} placeholder because seems to me it was incorrectly processed with CMake or Perl or something else (was cut off).
Now it works for Win32 as well as on Linux.

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 13, 2020

@snikulov Looks much better, but I don't yet see the arguments -a and -p being passed into runtests.pl.

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 13, 2020

@mback2k How can you see this?
See this. Target test-nonflaky will set those switches.
I execute exactly this target

@mback2k
Copy link
Member Author

@mback2k mback2k commented Oct 13, 2020

@snikulov ah, thank you very much. I missed that and already wondered if that target is CMake specific and how !flaky is passed.

snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests
Removed curly braces (${TFLAGS} -> $TFLAGS)
Move testdeps build to build step (per review comments)

Closes curl#6052
snikulov added a commit to snikulov/curl that referenced this issue Oct 13, 2020
Updated appveyor.yml to set env variable TFLAGS and run tests
Removed curly braces (${TFLAGS} -> $TFLAGS)
Move testdeps build to build step (per review comments)

Closes curl#6052
@mback2k mback2k closed this in d8fffd7 Oct 14, 2020
@snikulov
Copy link
Member

@snikulov snikulov commented Oct 14, 2020

@mback2k @bagder BTW, should we also enable tests on Travis CMake builds? After this change it's now possible.

@bagder
Copy link
Member

@bagder bagder commented Oct 14, 2020

Yes, we should!

@snikulov
Copy link
Member

@snikulov snikulov commented Oct 14, 2020

Ok then. Will submit another PR soon.

bagder added a commit that referenced this issue Oct 15, 2020
... and make TESTFAIL stand out a little better by adding newlines
before and after.

Reported-by: Marc Hörsken
Issue: #6052
Closes #6053
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.