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

Update tests to permit any order of output lines by sorting the editorconfig output #23

Merged
merged 5 commits into from
Nov 21, 2018

Conversation

cxw42
Copy link
Member

@cxw42 cxw42 commented Nov 18, 2018

@xuhdev @ppalaga Here we go again! :D

editorconfig-core-c and my editorconfig-core-vimscript both pass 100% of tests on Cygwin (edit) and Windows with this PR in place (except for utf-8 filenames).

Note: also includes one commit for the benefit of Cygwin (my development environment) to not run tests that can't possibly succeed on Cygwin.

ppalaga: Would you please test your Java CI?

Edit I have now run these tests on mingw-w64 x86 with Windows CMake 3.6.1 in cmd. All pass except for utf_8_char, which I didn't touch :) . I see per this (linked from editorconfig/editorconfig-core-c#31 (comment)) that this is a known issue on Windows.

@xuhdev xuhdev self-requested a review November 19, 2018 06:36
@xuhdev xuhdev added the bug label Nov 19, 2018
filetree/CMakeLists.txt Outdated Show resolved Hide resolved
Chris White added 3 commits November 20, 2018 20:45
Cygwin inherits Windows filesystem limitations
filetree now requires CMake 3.5+ so that Cygwin will be treated as a
non-Win32 platform.  See editorconfig#23 (comment)
by @xuhdev.
@cxw42
Copy link
Member Author

cxw42 commented Nov 21, 2018

@ppalaga Have you had a chance to try out the Java core? Unless this change causes problems for you, I think it's ready to merge.

Copy link
Member

@xuhdev xuhdev left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work, @cxw42 ! Except for the minor thing I pointed out, can you also make the coding style consistent? Sometimes you have spaces inside braces but sometimes you don't. Other than those, I think it's ready to merge.

CMakeLists.txt Outdated Show resolved Hide resolved
- Update copyright date to 2018
- Consistent formatting: no spaces inside the wrapping parans on CMake
  function calls
- Change EQUAL to STREQUAL where appropriate
@cxw42
Copy link
Member Author

cxw42 commented Nov 21, 2018

@xuhdev Fixed, plus I found and fixed a few EQUAL comparisons that should have been STREQUAL. I re-ran {C core, Vimscript core} x {Cygwin, MinGW} and it looks good. Thanks!

I updated the copyright dates across the board so no one else would have to do them for the rest of the year :) .

I removed the spaces inside the parens for consistency with the existing code.

@ppalaga
Copy link
Contributor

ppalaga commented Nov 21, 2018

There are 20 failing tests with ec4j:

Let me check what is the problem.

@ppalaga
Copy link
Contributor

ppalaga commented Nov 21, 2018

Here is the detailed output for the first failure meta_multiline:

2/189 Testing: meta_multiline
2/189 Test: meta_multiline
Command: "C:/Program Files (x86)/CMake/bin/cmake.exe" "-D" "EDITORCONFIG_CMD=C:/Program Files/Java/jdk1.7.0/bin/java.exe;-cp;C:/projects/ec4j/core/target/ec4j-core.jar;org.ec4j.core.cli.Cli" "-D" "ECARGS:LIST=-f;meta.in;C:/projects/ec4j/core/editorconfig-core-test/meta/meta.c" "-P" "C:/projects/ec4j/core/editorconfig-core-test/cmake/ec_sort.cmake"
Directory: C:/projects/ec4j/core/editorconfig-core-test/meta
"meta_multiline" start time: Nov 21 07:01 Coordinated Universal Time
Output:
----------------------------------------------------------
CMake Error at C:/projects/ec4j/core/editorconfig-core-test/cmake/ec_sort.cmake:55 (message):
  C:/Program
  Files/Java/jdk1.7.0/bin/java.exe;-cp;C:/projects/ec4j/core/target/ec4j-core.jar;org.ec4j.core.cli.Cli
  -f;meta.in;C:/projects/ec4j/core/editorconfig-core-test/meta/meta.c
  returned a nonzero exit code
<end of output>

source: https://ci.appveyor.com/project/ppalaga/ec4j/builds/20452747?fullLog=true#L894

Maybe the reason is that my EDITORCONFIG_CMD consists of java executable and some space separated arguments? Here is its definition:
https://github.com/ec4j/ec4j/blob/8b8fa5578e293bf2e209432f2a1a399ff23fe36f/core/CMakeLists.txt#L20

Should I change something there?

@ppalaga
Copy link
Contributor

ppalaga commented Nov 21, 2018

When I cd to the proper directory, replace semicolons with spaces in the command that returned a nonzero exit code above, and run it, it works properly. It returns answer=42 as expected.

I am not sure where the semicolons come from. Maybe they should not be there?

In run_and_sort, instead of taking a separate program and arguments,
take an undifferentiated command line.  This permits either
EDITORCONFIG_CMD or ECARGS to be either list- or string-valued.
@cxw42
Copy link
Member Author

cxw42 commented Nov 21, 2018

@ppalaga Thanks! Would you please try the new commit? Before doing so, in your Travis/Appveyor config, would you please add --output-on-failure to the ctest command line? That will give us a bit more information on failures.

The semicolons in .../java.exe;-cp;C:/projects/... are added by CMake. This is because the definition

set(EDITORCONFIG_CMD ${Java_JAVA_EXECUTABLE} -cp ${PROJECT_SOURCE_DIR}/target/ec4j-core.jar org.ec4j.core.cli.Cli)

creates a list-valued variable, and lists in CMake are just strings with semicolons between list items (reference). By contrast, set(CMD "${Java} -cp ${...}") (with "") would create a string-valued variable.

I have updated cmake/ec_sort.cmake to accept list-valued variables or single strings. It should now pass the tests using your current EDITORCONFIG_CMD. Please let me know. Much appreciated!

@ppalaga
Copy link
Contributor

ppalaga commented Nov 21, 2018

Yes, with eccf9d4 I see ec4j passing locally. Let's see if both CI jobs will pass too https://ci.appveyor.com/project/ppalaga/ec4j/builds/20462289 https://travis-ci.org/ec4j/ec4j/jobs/457960159

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

ec4j is passing with eccf9d4. This PR is good to merge.

@xuhdev xuhdev merged commit c87a301 into editorconfig:master Nov 21, 2018
xuhdev pushed a commit that referenced this pull request Nov 21, 2018
filetree now requires CMake 3.5+ so that Cygwin will be treated as a
non-Win32 platform.  See #23 (comment)
by @xuhdev.
@xuhdev
Copy link
Member

xuhdev commented Nov 21, 2018

Great work! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants