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

[WIP] make pipeline cleaner (Cont. #33) #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Oct 6, 2019

Cont. #33

In this PR, a "constant" global runtime flag TestMode is introduced so that it's easier to handle environment variables mentioned in #8

This may have many use cases, for example, in this PR I dispatch_create_new_reference and _update_reference on TestMode.

The first commit doesn't change the behavior of reference_test. I'd love to hear advice about what you'd like to have in further commits.

src/core.jl Outdated
GLOBAL_TESTMODE = _get_testmode()
end
return GLOBAL_TESTMODE
end
Copy link
Member Author

Choose a reason for hiding this comment

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

TestMode() generates a runtime singleton flag.
Please enlighten me if there's a best-practice as an alternative to this usage.

@coveralls
Copy link

coveralls commented Oct 6, 2019

Coverage Status

Coverage decreased (-3.9%) to 78.333% when pulling 6b3e903 on jc/pipeline into 46d650a on master.

@johnnychen94
Copy link
Member Author

The coverage decreases because CI can't capture codes for InteractiveMode. Any advice on how to test this?

@johnnychen94 johnnychen94 changed the title [WIP] introduce TestMode [WIP] make pipeline cleaner (Cont. #33) Oct 6, 2019
@johnnychen94
Copy link
Member Author

johnnychen94 commented Oct 12, 2019

I'm not satisfied with the current codes in this PR, but I don't have better ideas now.

One key question is: what advantages it brings to us if it doesn't only rely on isinteractive for triggering the input prompt? (#8 ) The only use case I can think about is to automatically generate all reference files(but not update) in a non-interactive mode (even when isinteractive()==true).

@oxinabox
Copy link
Member

I like this concept overall.

I think InteractiveMode is wrong name for this.
We have 3 behavours:

  • Fail: make the test fail
  • Create/Replace: just create the file or overwrite existing file
  • Prompt: ask the user

In all of them you get at least a warning and a display of the differences.

And those behavours can be applied in 2 circumstances:

  • Reference File Missing
  • Reference File Disagrees.

We can handle the combination via multiple dispatch.

There is a case to be made for making the default setting be:
Fail on Disagree, Create on Missing.
That way if you knew a bunch of things had changed,
you could delete all the ones you expect to be invalidated.
Then rerun the tests.

Or if you expect everything to be invalidated you just switch to Create on Both.

Having used this inpractice a bit, this is fine,
because one reviews the new referenced files during code-review.

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.

3 participants