Test suite: Permit editorconfig programs to provide correct output lines in any order

[cxw42]

At least the lowercase-value tests beginning here check for two or more lines of output in succession using ^(line1)(line2)$ (in essence). This requires that line1 and line2 be output in that order. (CMake regex reference)

However, I cannot find any documentation here, in the editorconfig-core-test issues, in #22 , on https://editorconfig.org, or in the Doxygen output, that specifies that requirement.

As far as I can tell, the test requires the options be output in the same order they are given in the .editorconfig file. Is that indeed a requirement? It seems somewhat brittle to me.

Would it be possible to change the spec and tests so that the output lines from the editorconfig program can appear in any order?

Edit

Adding both orders to the test case is a brute-force option, but it works. For example:

new_ec_test(lowercase_values1 lowercase_values.in test1.c
    "(^indent_style=space[ \t]*[\n\r]+end_of_line=crlf[ \t\n\r]*$)|(^end_of_line=crlf[ \t\n\r]+indent_style=space[ \t]*[\n\r]*$)"
    )                                                            #^ the part starting here is new

Edit 2 The linked commit is a better way to fix it, in my opinion - test for each line with a separate regex.

[xuhdev]

You are right they shouldn't be required. IMO this is indeed a bug in the test suite.

[cxw42]

Thanks! I'll finish working through the tests and then submit a PR in editorconfig-core-test, unless you have a different preference.

[xuhdev]

@cxw42 Thanks! Shall we give it a few days in case I may miss something? @editorconfig/developers am I right that currently no one is using the specific order of output by EditorConfig cores? Once the test cases change, we will loose the output by allowing different orderings of property value pairs.

[ppalaga]

I am not against the core idea of this proposal - i.e. to change the tests in such a way that the order in the returned set of properties is not significant. At the same time, I am against how the proposal is implemented in cxw42/editorconfig-core-test@99187fc . The strategy used there, namely

Any time you want to test for two different output lines, test them separately.

Substantially changes the semantics of the tests. Before the change, the returned results are often combined from multiple sections. After the tests the combination is not there and the combination logic in the tested editorconfig processors is not executed.

Please find a different way how to reach your goal.

[xuhdev]

I think @ppalaga 's comments make sense. In particular, you can modify new_ec_test to easily alter test cases.

[cxw42]

@ppalaga Thanks for your feedback! I am not sure I quite understand. Would you please look at new commit c82e1ab and see if it covers the case you are thinking of? For a target of test.file, it checks that [test.*] and [*.file] properties have been combined, but does so with a test per output line. If that commit indeed covers the situation you are concerned about, would you please give me a bit more detail about the failure mode that c82e1ab would not detect, but that the current tests would detect?

I agree that for tests of multiple files in one run (INI-format editorconfig output), a test per output line is insufficient. It might be worth narrowing the scope of this issue to tests of only one file.

I also agree that a test per output line is insufficient if an editorconfig command, when run twice with the same inputs, produces different outputs. However, I submit that any core implementation with that problem will likely fail more than just these tests :) .

That said, we can use the brute-force approach of (<order 1>|<order 2>|...|<order n>). However, that requires the test author to copy and edit the same regex n! times. I am concerned about the readability of such a test. Plus, people writing new tests might accidentally introduce typos while changing the order.

Currently, editorconfig-core-test requires CMake 2.6+ (according to its CMakeLists.txt). If we could bump that up to 3.0+, we would have more string- and list-manipulation support. With 3.0+, we might be able to generate all the different orders programmatically, which would remove the typo risk. We would just have to be careful about semicolons (CMake list separators), which I don't think would be an issue.

What say you all?

[ppalaga]

@cxw42 my vote was against splitting the tests as done in https://github.com/cxw42/editorconfig-core-test@99187fc . cxw42/editorconfig-core-test@c82e1ab adds a few tests, but the original tests are still split, and the general idea of banning tests returning multiple lines is still present, which I still do not find good.

brute-force approach of (<order 1>|<order 2>|...|).

Yes, that one is impractical.

If we could bump CMake up to 3.0+

I personally have no problem with that. Travis [1] has 3.9.2 and AppVeyor [2] has 3.11.0.

With 3.0+, we might be able to generate all the different orders programmatically

I hoped for this kind of solution.

[1] https://travis-ci.org/ec4j/ec4j/jobs/369715134#L66
[2] https://ci.appveyor.com/project/ppalaga/ec4j/build/1.0.60?fullLog=true#L9

[cxw42]

@ppalaga I'll bump to 3.0 and see about auto-generating the factorial combination.

@xuhdev It will definitely take me a little while to work on this :) . I will reply again when I have something ready for everyone's review.

[cxw42]

@ppalaga Two things that just occurred to me before I go off and write code that will generate 120 permutations in one test to replace the five tests in c82e1ab.

So that I understand where you're coming from, is your objection philosophical or based on a failure case you've seen before? I'd still be interested to see an incorrect core that passes under test-per-line (other than a core deliberately written to output individual lines in the order expected by the tests!).

1

Would you be comfortable with the one-line-per-test approach if we also tested against the expected number of lines of output? For example, if editorconfig should output

a=1
b=2

then use three tests:

• (^|[\n\r]+)a=1[ \t\n\r]+
• (^|[\n\r]+)b=2[ \t\n\r]+
• (new one) ^[^=]+=[^=]+=[^=]+$

That is, test that there are exactly two lines of output (the new one; keys off the = chars), and that the two lines that we want both appear.

2

In CMake 3.0.2+, we can use execute_process() to capture stdout. With a nested cmake or ctest invocation, maybe we could capture that output and then test multiple regexes against the single captured output.

[ppalaga]

is your objection philosophical or based on a failure case you've seen before?

I do not remember any particular failure like that. I am a co-author of an EditorConfig parser written in Java that is tested using the core test suite and I occasionally contribute to the test suite.

I understand your original proposal so that it imposes an informal policy for the current and future tests that at most one returned line can be tested per test. To reach that, you either have to (a) change the output of the given test (thus changing the semantics) or (b) split the tests thus worsening maintainability of the tests. I admit I have not looked properly if you did (a) or (b) and I am sorry for that. I think both (a) and (b) is bad. I prefer such a solution that allows for writing concise and correct tests.

Googling for who is using the suite [1] reveals that there are at least 10 implementations there (
lua, c#, c, go, 2 x java, js, emacs, python, ruby). All those people need to read an understand the tests. And I think all those people are susceptible to submit new tests that will break the informal policy regarding the insignificancy of the returned properties order because it simply works for them either way. I therefore vote for a solution that keeps it easy to write the tests.

I hope for something like

new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig any_order("keyb=valueb", "keyc=valuec"))

instead of the current

new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig "^keyb=valueb[ \t\n\r]*keyc=valuec[ \t\n\r]*$")

I have no idea whether this is possible with the cmake scripting.

[1] https://www.google.com/search?gl=us&hl=en&ei=B4_JW4bjM4mZgAb8oKrwDw&q=gitmodules+github.com%2Feditorconfig%2Feditorconfig-core-test.git+site%3Agithub.com&oq=gitmodules+github.com%2Feditorconfig%2Feditorconfig-core-test.git+site%3Agithub.com&gs_l=psy-ab.3...56050.63600..63879...0.0..0.74.951.15......0....1j2..gws-wiz.......0i71.wAm36_rQBY0

[ppalaga]

Or even better change new_ec_test to accept an array and handle the ordering internally:

new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig ["keyb=valueb", "keyc=valuec"])

[hgraeber]

Hi Peter! Am 2018-10-19 um 10:24 schrieb Peter Palaga:

[...] I hope for something like |new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig any_order("keyb=valueb", "keyc=valuec")) | instead of the current |new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig "^keyb=valueb[ \t\n\r]*keyc=valuec[ \t\n\r]*$") | I have no idea whether this is possible with the cmake scripting.

No, It's not. But cmake parameter lists can have arbitary length: |new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig "^keyb=valueb[ \t\n\r]*$" "^keyc=valuec[ \t\n\r]*$")| Adittionally it is possible to have self-defined keywords in the parameter list, something like named parameters. Those or by convention completely uppercase. This may look like |new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig REGEXP "^keyb=valueb|||[ \t\n\r]*$|" "^keyc=valuec||||[ \t\n\r]*$|| "))| The keyword is not really needed, but its use makes it easier to extend the parameter list of|new_ec_test |further if necessary. For example for full matches without regexp for easy cases: |new_ec_test(star_matches_dot_file_after_slash star.in Bar/.editorconfig MATCH "keyb=valueb||||" "keyc=valuec"||))| Maybe it is useful to add other keywords, too: |new_ec_test(TEST star_matches_dot_file_after_slash FILE star.in EDITORCONFIG_FILE Bar/.editorconfig MATCH "keyb=valueb||||" "keyc=valuec"||))| Herbert

[cxw42]

@ppalaga Thanks for the explanation! I understand communication challenges, and why those challenges cause you to prefer keeping the existing structure. I am almost done with a core in native VimScript (to replace the current Vim plugin+Py core), so the number of projects to communicate among will be even higher in the near future :) .

@hgraeber Thanks for pointing out those options! I will start with the simple case (arbitrary number of regexes) and see about more flexible argument handling later if appropriate based on feedback.

[cxw42]

All, my next take is in the anyorder branch. On Cygwin, the C core and my Vimscript core* pass all the tests in this branch. Here's the comparison. In short:

• New test function ec_test_lines() sets up a test that will match the given regex lines in any order. Example call:

  ec_test_lines(NAME parent_and_current_dir 
                EC_FILE parent_directory.in 
                SRC_FILE parent_directory/test.b
                REGEXES "key1=value1[ \t]*" "key2=value2[ \t]*")
  

  looks for a key1=value lines followed by a key2=value2 line, or vice versa.

• Helper file cmake/multiline_matcher.cmake defines function make_regex_of_permutations() that generates a single regex of all the permutations.

Shall I submit a PR now and move the discussion there, or would you prefer to continue on this thread? Also, I can squash before submitting the PR if you wish, or you can squash later.

Note: I decided to require CMake 3.1.0 instead of CMake 3.0.0. This is because 3.1.0 introduces the sane argument handling in if() conditionals. I don't feel like enough of a CMake expert to avoid subtle bugs in conditionals under the old rules. @ppalaga, as you mentioned earlier, CI is at least 3.9.x, so I don't think this should be a problem.

Thanks!

* Except that the Vimscript core doesn't yet support the single UTF-8 test, and doesn't run the tests of the behaviour noted in #371.

[ppalaga]

ec_test_lines(NAME parent_and_current_dir 
              EC_FILE parent_directory.in 
              SRC_FILE parent_directory/test.b
              REGEXES "key1=value1[ \t]*" "key2=value2[ \t]*")

Thanks, I like this. Just maybe replace REGEXES with EXPECTED.

[cxw42]

@ppalaga Done!

All, one thing I forgot to mention - the current version is limited to three lines. That is because:

• CMake regexes are limited to nine capture groups
• CMake regexes don't have non-capturing groups
• To match the beginning of a line, I have to use a group: (^|[\n\r]+).

Does that concern anyone? I could permit more than three lines if we also added an option to the editorconfig to output a blank line before any other output, because then start-of-line would just be [\n\r]+ and wouldn't require the group.

[ppalaga]

the current version is limited to three

Assuming there is no test in the suite nowadays working with more than three lines, what happens when somebody uses more than three expressions in EXPECTED in the future? I do not mind the limitation, if the test execution fails with a clear message in such cases.

[cxw42]

@ppalaga Yep, it dies with a very blatant error message :) . See here.

I'll squash and open a PR in the next day or two.

[cxw42]

All, PR editorconfig/editorconfig-core-test#22 is open for your consideration. Thanks!

[cxw42]

@xuhdev editorconfig/editorconfig-core-test#22 is ready for merging per @ppalaga. It looks like you are the most common committer in that repo - do you need anything else from me before merging? Are you comfortable with the changes? Thanks!

Edit for later readers - editorconfig/editorconfig-core-test#22 was closed at the maintainers' request. A different PR will be coming soon!