Skip to content

simplified test file skipping (#4996) #5003

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Mar 13, 2018
Merged

simplified test file skipping (#4996) #5003

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c96f07e
simplified test file skiping (#4996)
rdeforest Mar 3, 2018
8188dda
name skipped files explicitly, inline skip test
rdeforest Mar 9, 2018
53abbe9
typo (fileName vs filename)
rdeforest Mar 9, 2018
4c6fe65
Edit comment for public consumption, improve filename filter preparation
rdeforest Mar 10, 2018
7774864
Minor syntax surprise
rdeforest Mar 10, 2018
0661ae8
if not -> unless
rdeforest Mar 10, 2018
26391af
Per feedback: make skipUnless' 2nd arg non-slurpy
rdeforest Mar 11, 2018
2f4ebde
typo (always run tests before committing!)
rdeforest Mar 11, 2018
3fc6d46
Only do feature detection when we're running tests; fix feature detec…
GeoffreyBooth Mar 13, 2018
2320b6f
Fix references to now-deleted `global.supportsAsync`
GeoffreyBooth Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions Cakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ header = """
# Used in folder names like `docs/v1`.
majorVersion = parseInt CoffeeScript.VERSION.split('.')[0], 10

# CoffeeScript supports a range of major versions of NodeJS. Major versions of
# NodeJS differ in the features they support. CoffeeScript features which
# depend on features which are not available across all supported NodeJS
# versions need only be tested against versions of NodeJS which provide the
# required feature. The purpose of the `testFilesToSkip` array is to exempt
# files which test features not available within the NodeJS runtime currently
# under test.
testFilesToSkip = []

# 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...) ->
unless (try new Function code)
testFilesToSkip = testFilesToSkip.concat names

skipUnless 'async () => {}', 'async.coffee'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


# Log a message with a color.
log = (message, color, explanation) ->
Expand Down Expand Up @@ -425,12 +443,6 @@ runTests = (CoffeeScript) ->
catch err
onFail description, fn, err

global.supportsAsync = try
new Function('async () => {}')()
yes
catch
no

helpers.extend global, require './test/support/helpers'

# When all the tests have run, collect and print errors.
Expand All @@ -450,8 +462,7 @@ runTests = (CoffeeScript) ->

# Run every test in the `test` folder, recording failures.
files = fs.readdirSync 'test'
unless global.supportsAsync # Except for async tests, if async isn’t supported.
files = files.filter (filename) -> filename isnt 'async.coffee'
.filter (filename) -> filename not in testFilesToSkip

startTime = Date.now()
for file in files when helpers.isCoffee file
Expand Down