simplified test file skipping (#4996) #5003

rdeforest · 2018-03-04T17:38:28Z

This is a re-creation of PR #4998, which automatically closed because I deleted the branch it came from, and was itself a re-creation of PR #4997 without the extraneous history. Original description follows.

Per discussion on #4893 and #4997, managing exclusion of tests when newer features are required is currently non-obvious. The goal of this PR is to make maintenance of such exceptions easy and surprise-free.

The existing method was a hard-wired exception for the file 'async.coffee'. When I added async_iterators for #4893 I was surprised because I didn't know about the build:watch:harmony build target nor the hardwired exception in runTests. Now both the feature test and filename match are handled on the same lines near the top of Cakefile.

I included two currently unused pattern/feature pairs to demonstrate the intent and value of this approach. Both will likely be used for #4877 and #4893.

I didn't write a test for this change since it's a change to the test system.

After posting this I'm going to figure out how to remove the extra unrelated empty commits, but I wanted to get this out there before the weekend to follow through on my previous commitment.

rdeforest · 2018-03-04T17:53:01Z

I started looking through other PRs and noticed that #4880 would benefit from this feature as well. The tests which need to be skipped would have to be in their own file though. Supposing it were called node9_regexps.coffee, the updated table would look something like

testFilesToSkip = [
  /async/           unless try new Function 'async ()   => {}'
  /async_iterators/ unless try new Function 'async () * => {}'
  /exponent_ops/    unless try new Function 'x = 3; x **= 2 ** 2; return x === 81'
  /node9/           unless process.version.match /^v9/
].filter _.identity

(I wonder why the GitHub coffee colorizer is confused by this code? Vim gets it right.)

rdeforest · 2018-03-04T19:24:24Z

#4884 seems to want this too.

rdeforest · 2018-03-06T17:33:27Z

While I'm confident this patch is a step forward, I've been working on a more elegant approach.

My current thinking is similar to what I proposed on another thread, where each set of tests with the same requirements goes in its own directory. Instead of having a config file, index.coffee in each directory handles the should these tests run question.

Edit to add: #5007 is my more elegant solution and obsoletes this PR if it is accepted.

zdenko · 2018-03-07T12:29:35Z

I think this PR has better, easier solution for skipping specific tests. Organizing files in folders seem to much work. In most cases, there would be just one file (+ index.coffee) per folder.

This got me thinking if there could be an even better solution to skip tests. Instead of relying on the file name pattern (or folders) we could use custom identifiers. What if sometimes test needs to pass more then one check, e.g. async generators and object rest/spread?
So, here's my quick proposal (just brainstorming).

We could add an array of conditions per test function:

test "async as argument", ["async", "some_other_condition"], ->
  ok ->
    await winning()

(The array is placed after the description because some functions are quite long and I think it is more readable this way.)

The array of test conditions identifiers is similar as in this PR but contains conditions which passed the tests.

testFilesConditons = [
  /async/           if try new Function 'async ()   => {}'
  /async_iterators/ if try new Function 'async () * => {}'
  /exponent_ops/    if try new Function 'x = 3; x **= 2 ** 2; return x === 81'
].filter _.identity

featuresPresentFor = (cond) -> testFilesConditons.find (filePattern) -> cond.match filePattern

Then, in global.test, we check if the function has conditions and if any of them fails we skip the test.

# Our test helper function for delimiting different test cases.
global.test = (description, fnCond, fn = no) ->
  canTest = (conditions) ->
    passed = (yes for cond in conditions when featuresPresentFor(cond)?)
    passed.length is conditions.length

  try
    fn = fnCond if _.isFunction fnCond
    fn.test = {description, currentFile}
    # If the test has specific conditions (i.e. `async generators`) 
    # and doesn't pass, it will be skipped.
    if _.isArray(fnCond) and not canTest fnCond
      result = yes
    else
      result = fn.call(fn)
...

I tried this approach locally and it seems to be working.

GeoffreyBooth · 2018-03-07T22:51:38Z

I agree with @zdenko that I’d rather a Cakefile-based approach than a folder-based approach. It’s not that hard to edit the Cakefile, and we may have to for future oddball things like more attachments to global such as testAsync.

@zdenko I don’t think your suggestion will work. Use the tool n (npm install -g n) to switch to an older version of Node, like Node 6, and you’ll see why. Even if a particular function or block of code with new syntax never gets executed, for example if it’s inside an if supportsAsync block, the runtime still parses that never-to-be-executed code, and the parsing itself throws an error. That’s why we need to do these contortions to prevent unsupported syntax from ever being parsed, by preventing Node from ever loading the files in the first place; or by treating the experimental syntax as a string to be evaled.

GeoffreyBooth · 2018-03-07T23:54:55Z

Cakefile

@@ -29,6 +29,15 @@ header = """
 # Used in folder names like `docs/v1`.
 majorVersion = parseInt CoffeeScript.VERSION.split('.')[0], 10

+# Patterns of names of test files which depend on currently absent features
+testFilesToSkip = [
+  /async/           unless try new Function 'async ()   => {}'


Shouldn’t this be /^async$/ so that it doesn’t automatically include async_iterators as well?

My reasoning was that any file with 'async' in its name probably requires the async feature, but I don't mind being more specific if you prefer.

I think we should map feature detections to specific files, rather than relying on substrings in filenames. At quick glance it’s not obvious that /async/ will match both async.coffee and async_iterators.coffee, and so this is a potential bug in the future when someone adds another one that overlaps with filenames they didn’t intend. I think all you need to do is change these regexes into strings? And then change fileName.match to be an equality check.

GeoffreyBooth · 2018-03-07T23:56:14Z

Cakefile

+  /exponent_ops/    unless try new Function 'x = 3; x **= 2 ** 2; return x === 81'
+].filter _.identity
+
+featuresPresentFor = (fileName) ->


Can you please explain the name of this variable? Why create this function as a separate variable at all, rather than an inline argument to filter below (similar to the old code)?

I was thinking of it in terms of being a predicate.

if featuresPresentFor someFile then process someFile

But you're right that it looks weird when passed to filter. Perhaps it should be "fileCanRun"? I'm open to other suggestions.

Maybe just put the function inline next to filter? Then it doesn’t need a name.

zdenko · 2018-03-08T18:53:23Z

@GeoffreyBooth you're right of course. It won't work. In my experiment, I used simple test function which passed on Node 6.

rdeforest · 2018-03-09T23:29:42Z

whoops. I should have run the tests before pushing... jussec

GeoffreyBooth · 2018-03-10T00:11:55Z

Cakefile

+  'async.coffee'           unless try new Function 'async ()   => {}'
+  'async_iterators.coffee' unless try new Function 'async () * => {}'
+  'exponent_ops.coffee'    unless try new Function 'x = 3; x **= 2 ** 2; return x === 81'
+].filter _.identity


Is the purpose of this is just to prevent undefined elements in the array? But such elements shouldn’t matter in filename not in testFilesToSkip. I know this is a bit nitpicky of an optimization, but I was scratching my head wondering what filter _.identity was doing 😄

Also please put a period at the end of your comment. The comments become the documentation in the annotated source, so they should be complete sentences.

When the list was treated as a list of RegExps to be String::match'd against, undefined was unwelcome in the list. Now it's fine to leave them in. This is why we have code reviews. :)

I also didn't realize the Cakefile was subjected to the same comment -> documentation treatment as the files in src/. I'm adjusting my comments to account for this. Update forthcoming...

GeoffreyBooth · 2018-03-10T21:45:54Z

Cakefile

+# The `skipUnless` function adds file names to the `testFilesToSkip` array
+# when the feature(s) the file(s) depend on are not available.
+skipUnless = (code, names...) ->
+  if not (try new Function code)


This can be unless (try new Function code)

GeoffreyBooth · 2018-03-10T21:49:18Z

Cakefile

+  if not (try new Function code)
+    testFilesToSkip = testFilesToSkip.concat names
+
+skipUnless 'async ()   => {}',                     'async.coffee'


Technically, if async functions aren’t supported, we should be skipping async.coffee and async_iterators.coffee both. It’s irrelevant in this case because you have the second check below, but if we’re thinking of this code as being self-documenting—i.e., ”the files to skip when this test fails are…”—then the second argument should be ['async.coffee', 'async_iterators.coffee'].

Also I’m not sure how scalable this style is, when we add a test that isn’t < 60 characters or so, but we can worry about that when that day comes.

It's easy to re-factor for widely-applicable tests

asyncTestFiles = (files...) -> skipUnless 'async () = {}', files... asyncTestFiles 'async.coffee', 'async_iterator.coffee' asyncTestFiles 'some_other_async_test.coffee'

Or we could get really wild...

asyncTest = 'async () -> {}' appendDotCoffee = (s) -> s + ".coffee" asyncPrefixes = (s) -> skipUnless asyncTest, s.split(/\s+/).map(appendCoffee)... asyncPrefixes """ async async_iterators some_other_async_test """

Can you tell I <3 CoffeeScript? :)

Can you tell I <3 CoffeeScript? :)

Indeed. 😄 We try to make the CoffeeScript codebase a “best practices” example of how to write CoffeeScript, so code golf isn’t exactly what we’re going for. 😄 I think something as simple as making the second argument an array is probably sufficient.

That's my Perl roots showing. Sorry about that. :) As you said, we can cross that bridge when we come to it.

GeoffreyBooth · 2018-03-11T21:07:03Z

Cakefile

+
+skipUnless 'async ()   => {}',                     'async.coffee'
+skipUnless 'async () * => {}',                     'async_iterators.coffee'
+skipUnless 'x = 3; x **= 2 ** 2; return x === 81', 'exponent_ops.coffee'


@rdeforest If you don’t mind just changing this section so that the second parameters are arrays, and the first one is both async.coffee and async_iterators.coffee, I think this can be merged in.

Done and done. I also made the third test multi-line to serve as an example. I hope you agree it's an improvement.

…tions; update style

GeoffreyBooth · 2018-03-13T02:08:54Z

Thanks @rdeforest!

simplified test file skiping (jashkenas#4996)

c96f07e

rdeforest mentioned this pull request Mar 6, 2018

Feature test directories (alternative to PR 5003) #5007

Closed

GeoffreyBooth reviewed Mar 7, 2018

View reviewed changes

name skipped files explicitly, inline skip test

8188dda

typo (fileName vs filename)

53abbe9

GeoffreyBooth reviewed Mar 10, 2018

View reviewed changes

rdeforest added 2 commits March 9, 2018 20:14

Edit comment for public consumption, improve filename filter preparation

4c6fe65

Minor syntax surprise

7774864

GeoffreyBooth reviewed Mar 10, 2018

View reviewed changes

if not -> unless

0661ae8

GeoffreyBooth reviewed Mar 10, 2018

View reviewed changes

GeoffreyBooth reviewed Mar 11, 2018

View reviewed changes

rdeforest and others added 4 commits March 11, 2018 15:39

Per feedback: make skipUnless' 2nd arg non-slurpy

26391af

typo (always run tests before committing!)

2f4ebde

Only do feature detection when we're running tests; fix feature detec…

3fc6d46

…tions; update style

Fix references to now-deleted global.supportsAsync

2320b6f

GeoffreyBooth merged commit 5a43b2d into jashkenas:master Mar 13, 2018

GeoffreyBooth mentioned this pull request Mar 13, 2018

Proposal: generalize exempting of tests requiring advanced V8 features #4996

Closed

rdeforest deleted the master-pending-4998 branch March 13, 2018 17:33

GeoffreyBooth mentioned this pull request Mar 16, 2018

Fix #4877: Exponentiation operators #4881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplified test file skipping (#4996) #5003

simplified test file skipping (#4996) #5003

rdeforest commented Mar 4, 2018

rdeforest commented Mar 4, 2018

rdeforest commented Mar 4, 2018

rdeforest commented Mar 6, 2018 •

edited

Loading

zdenko commented Mar 7, 2018

GeoffreyBooth commented Mar 7, 2018

GeoffreyBooth Mar 7, 2018

rdeforest Mar 9, 2018

GeoffreyBooth Mar 9, 2018

GeoffreyBooth Mar 7, 2018

rdeforest Mar 9, 2018

GeoffreyBooth Mar 9, 2018

zdenko commented Mar 8, 2018

rdeforest commented Mar 9, 2018

GeoffreyBooth Mar 10, 2018

rdeforest Mar 10, 2018

GeoffreyBooth Mar 10, 2018

rdeforest Mar 10, 2018

GeoffreyBooth Mar 10, 2018

rdeforest Mar 11, 2018

GeoffreyBooth Mar 11, 2018

rdeforest Mar 11, 2018

GeoffreyBooth Mar 11, 2018

rdeforest Mar 11, 2018

GeoffreyBooth commented Mar 13, 2018

simplified test file skipping (#4996) #5003

simplified test file skipping (#4996) #5003

Conversation

rdeforest commented Mar 4, 2018

rdeforest commented Mar 4, 2018

rdeforest commented Mar 4, 2018

rdeforest commented Mar 6, 2018 • edited Loading

zdenko commented Mar 7, 2018

GeoffreyBooth commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdenko commented Mar 8, 2018

rdeforest commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Mar 13, 2018

rdeforest commented Mar 6, 2018 •

edited

Loading