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

Filter test inputs before feeding them to functions under test #3

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

1082008
Copy link

@1082008 1082008 commented Apr 26, 2017

This PR filters out noise from test input mentioned in #2

@cryptix
Copy link

cryptix commented Apr 27, 2017

Can you also commit the generated data.json?

I tried to run node generate.js but it hangs somehow after ok 27 (unnamed assert).

0.6 and 0.8 is not supported anymore.
@1082008
Copy link
Author

1082008 commented Apr 27, 2017

I could commit data.json but it is not a source file, therefore it shouldn't be committed in the source control system. I will make travis CI to put it in a build artifact but that needs some work.

Until then data.json is available here.

@cryptix
Copy link

cryptix commented Apr 28, 2017

it is not a source file

sorry, I don't get that. It is a very static file.

If we don't include it here we have to have nodejs with all the dependencies present in all test environments.

@1082008
Copy link
Author

1082008 commented Apr 28, 2017

It is a very big, nearly static file. I see it is easier to include it in source control, but I think we should consider that topic. I open a new issue but commit data.json for now as you requested.

Please note that this is generated using a modified clear method which
deletes the nullified properties. This ensures that noise is filtered also
from the output besides the input is clean. There is an open PR for that
on the secret-handshake project.

This is not a source file as it is generated by other source files.
Inclusion of data.json in source control should be reconsidered.
An issue will be opened for this.
Copy link
Author

@1082008 1082008 left a comment

Choose a reason for hiding this comment

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

@1082008
Copy link
Author

1082008 commented Apr 28, 2017

The problem of including data.json in source control is moved to #4

args[0] = saved_filtered_input_state // revert the changes fn made on its input
output(clone({name: name, args: args, result: result}))
args[0] = saved_input_state
return fn.apply(null, args)
Copy link
Author

@1082008 1082008 May 5, 2017

Choose a reason for hiding this comment

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

Calling fn twice causes duplicate call of verifyChallenge() on client side because clientVerifyChallenge() calls verifyChallenge()

@dominictarr
Copy link
Contributor

It's way simpler to just check it in. What we should do, though, is make the tests fully deterministic, (i.e. pass in random seeds for every test), then it will always be the same data.json.

@1082008
Copy link
Author

1082008 commented May 7, 2017

I agree, tests should be deterministic. Every time I run generate.js it generates a different data.json. This behavior causes a lot of noise in the diff.

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.

4 participants