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

Accept null or undefined as value #53

Open
isaacaggrey opened this issue Mar 13, 2018 · 2 comments · May be fixed by #58
Open

Accept null or undefined as value #53

isaacaggrey opened this issue Mar 13, 2018 · 2 comments · May be fixed by #58
Labels

Comments

@isaacaggrey
Copy link

isaacaggrey commented Mar 13, 2018

Current behavior:

unroll('should be okay with id being #id', (done, testArgs) => {
    let object = { id: testArgs.id };
    expect(object.id).toEqual(testArgs.id);
    done();
},
`
    where:
    id
    ${null}
    ${undefined}
`
);

// Output:
     encountered a declaration exception
    Error: title not expanded as incorrect args passed in

Expected behavior:

✔ should be okay with id being null
✔ should be okay with id being undefined

Granted, I understand with the dataTable method this is a bit tricky since it is a template literal (i.e., what if someone wants to test the string "null" or "undefined", not sure what is expected behavior), but I think the dataArray method should be valid.

@lawrencec lawrencec added the bug label Apr 7, 2018
@lawrencec
Copy link
Owner

Hey @isaacaggrey, Thanks for reporting the bug!

I agree this should be supported. I have a fix for both the array and datatable notations using Symbols. So for example, your dataTable example will look like this:

unroll('should be okay with id being #id', (done, testArgs) => {
    let object = { id: testArgs.id };
    expect(object.id).toEqual(testArgs.id);
    done();
},
`
    where:
    id
    ${Symbol.keyFor(unroll.NULL)}
    ${Symbol.keyFor(unroll.UNDEFINED)}
`
);

and the array notation:

unroll('should be okay with id being #id', (done, testArgs) => {
    let object = { id: testArgs.id };
    expect(object.id).toEqual(testArgs.id);
    done();
},
[
   ['id']
   [null],
   [undefined],
   [Unroll.NULL],
   [Unroll.UNDEFINED],
]

Admittedly, the dataTable example is ugly (though one could argue it's a little messy anyway cos of the need for JSON.stringify), this is because unroll needs to know that the undefined is intentional so a placeholder replacement has to be used as an identifier. And I'd like to keep functional parity between the two notations; I wouldn't want to be able to use null and undefined in array notation but not dataTables.

But I don't think there is an easier way of representing undefined or null` without removing the check (and subsequent thrown error) that the test title has not been expanded properly. The alternative is to get rid of the error checking but I think that's useful for the developer.

Would be interested in your thoughts and preferences as an end user. AFAIK, I'm the main user of the util so would be good to get other users opinion before I push this.

@Blackbaud-JasonBodnar
Copy link

@lawrencec any plans to merge this PR?

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 a pull request may close this issue.

3 participants