Skip to content

[SEC-180] Add ability to run tests for specific OS based on tags #1929

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

Conversation

NathanJPhillips
Copy link
Contributor

This facilitates tests for OS specific features as well as allowing tests that use syntax specific to the OS such as passing OS-specific paths or using OS-specific class path delimiters (semicolon on Windows, colon on other systems)

@tautschnig
Copy link
Collaborator

@NathanJPhillips Thank you for factoring this out!

Platform-specific tests are needed in places, but ideally as few of them as possible. There are two reasons I am showing limited appreciation of the proposed solution.

  1. There are discussions in Travis test speedups #1907 and elsewhere about deprecating test.pl. Adding more features will make that harder.
  2. Why would the different platforms (ever) be the right ones to distinguish? For example, sometimes mingw will behave the same as Visual Studio, at other times it won't.

Recommended reading: http://www.sourceware.org/autobook/ - yes, a lot of my resistance towards platform-specific bits stems from having read that book, the remaining ones from being a Debian Developer. As such I want to make writing platform-specific code harder, not easier. You don't know what platform your users are on, and they are the ones you should care about.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

As outlined in the comment below, I'd prefer to leave inference of the build environment to the build tools. Hence I'm not going to approve this one; but also I might be very narrow-minded, and someone else might say that the proposed solution is the way to go. Thus I'd leave it to either someone else to approve or to @NathanJPhillips to come up with a solution that suits my taste.

@@ -91,6 +91,7 @@ ($$$$$$$$$)
# not given (implying run all tests) or it matches at least one include-tag.

my $include_tags_length = @$include_tags;
# If any include tags are passed then only include tests including those tags
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part of the change is great.

if(not exists($tags_set{"OS-" . $os}) and grep { "OS-" eq substr($_, 0, length("OS-")) } @tags) {
$passes_tag_tests = 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not confident about this one. The surrounding environment has already solved this problem: CMake surely comes up with some identifier of the operating system (here, the crucial one will be WIN32), and we've got a solution for Make in src/common.

@smowton
Copy link
Contributor

smowton commented Mar 13, 2018

@tautschnig there's no build tool here to spot the environment, but how about instead:

CORE
some.class
--function x.f --classpath `platform_specific_classpath.sh a:b:c`

Where platform_specific_classpath.sh does the substitution as necessary?

@tautschnig
Copy link
Collaborator

There's also the approach proposed in #1915: generate the *.desc files at test-execution time.

@NathanJPhillips NathanJPhillips changed the title Add ability to run tests for specific OS based on tags [SEC-180] Add ability to run tests for specific OS based on tags Mar 21, 2018
@NathanJPhillips
Copy link
Contributor Author

@smowton's method implemented in #1816 so this is no longer necessary.

@NathanJPhillips NathanJPhillips deleted the feature/os-specific-tests branch March 27, 2018 17:11
@NathanJPhillips NathanJPhillips restored the feature/os-specific-tests branch March 27, 2018 17:11
@NathanJPhillips NathanJPhillips deleted the feature/os-specific-tests branch August 27, 2019 16:09
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.

3 participants