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

fix(assert): Add default name for no test name. #27

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alvaropinot
Copy link

First of all I'm a big fan of tape! Thanks for that 😄

I just discovered playing around with a custom TAP reporter for COBOL, (yep COBOL 😅) that whenever a TAP report has no name after the test or the name has no space faucet fails to parse the line.

For example given the following TAP as input:

TAP version 13
1..1
ok 1

or (without the space)

TAP version 13
1..1
ok 1foo
/Users/alvaropinot/work/alvaropinot/faucet/index.js:38
            var s = trim(res.name.trim());
                                 ^

TypeError: Cannot read property 'trim' of undefined
    at Parser.<anonymous> (/Users/alvaropinot/work/alvaropinot/faucet/index.js:38:34)
    at emitOne (events.js:101:20)
    at Parser.emit (events.js:191:7)
    at Parser._online (/Users/alvaropinot/work/alvaropinot/faucet/node_modules/tap-parser/index.js:131:14)
    at Parser._write (/Users/alvaropinot/work/alvaropinot/faucet/node_modules/tap-parser/index.js:53:14)
    at doWrite (/Users/alvaropinot/work/alvaropinot/faucet/node_modules/readable-stream/lib/_stream_writable.js:279:12)
    at writeOrBuffer (/Users/alvaropinot/work/alvaropinot/faucet/node_modules/readable-stream/lib/_stream_writable.js:266:5)
    at Parser.Writable.write (/Users/alvaropinot/work/alvaropinot/faucet/node_modules/readable-stream/lib/_stream_writable.js:211:11)
    at Stream.method [as write] (/Users/alvaropinot/work/alvaropinot/faucet/node_modules/duplexer/index.js:47:39)
    at Socket.ondata (_stream_readable.js:557:20)

This is a fix that prevents the problem but maybe the tap-parser should be fixed instead. I tried to follow the ES5 syntax and already existing style but please feel free to let me know any code standard/style that you want me to follow, if you want to merge this PR.

I was willing to add both scenarios as tests but looks like there is none 😅, quite ironic being a test output lib 😜

Can you confirm if there is any other implications about changing https://github.com/substack/faucet/blob/0fabf821eb142dc75a318c5cd408d7653279bb00/index.js#L38 looks like name is used in some other lines 😄


This are the proposed changes 👇

fix(assert): Add default name for no test name.

  • Add DEFAULT_TEST_NAME.
  • Ensure res.name has a value or use DEFAULT_TEST_NAME.

 * Add `DEFAULT_TEST_NAME`.
 * Ensure `res.name` has a value or use `DEFAULT_TEST_NAME`.
@ljharb
Copy link
Collaborator

ljharb commented Sep 22, 2022

@alvaropinot can you elaborate on what test code produces this output? in latest tape, i get # (anonymous) when i pass null or the empty string as the test name.

@ljharb ljharb marked this pull request as draft September 22, 2022 21:28
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.

2 participants