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

Handle directories in tests folder. Fixes #138 #146

Closed
wants to merge 2 commits into from

Conversation

KentonWhite
Copy link

Watchers use digest to efficiently check if a file has changed.
Digest does not handle missing files or directories well. So safe_digest
was written to wrap digest and catch errors.

In b1cb9e8 it was discovered that
the error catching was too optimistic and was eating errors that should
be reported to the user. To fix this, logic was added to catch
the known errors from digest and re-raise all other errors.

The only known error at that time was if the file did not exist.
Unfortunately digest also throws an error if it is passed a directory.
With the introduction of the new testthat tests folder structure,
all tests folders also include a subfolder called testthat. When
digest was passed the folder name it was failing.

Rather than filtering for folders, I've expanded the known errors logic.
The list of known errors is now encapsulated in a known_errors function.
An error is checked against this list, and if it isn't found then the
error is re-raised.

I also ran into problems with the tests in test-watcher. test-watcher
was explicitly testing internal functions, and I was having trouble
with the testing environment not finding these functions. I explicitly
export these functions if they are not in the current testing environment.
This is probably stupid and would love some pointers as to a better way
to test!

# In an interactive environment
# Get these functions from the testthat namespace

if(!exists('dir_state')) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what you're doing here

Copy link
Author

Choose a reason for hiding this comment

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

Both dir_state and compare_state are not exported. Running the tests locally, I was getting errors since they weren't in the global name space. This is my kludge to get the testing to work: if the function is not available in the global namespace, get the function from the testthat namespace. There is probably a better way to do this!

Copy link
Member

Choose a reason for hiding this comment

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

You can use devtools::test(, "test-watcher")) to run the tests in a single file. That's the easiest way to make sure the tests are run in the correct environment.

Copy link
Author

Choose a reason for hiding this comment

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

Great! That works. Pulling the confusing code out.

@KentonWhite
Copy link
Author

I've removed the confusing lines from the test. Was that the only issue that came up in code review?

@hadley
Copy link
Member

hadley commented Jun 23, 2014

Looks good. Could you please also add a bullet to the NEWS referencing the relevant issue and your github username.

Watchers use digest to efficiently check if a file has changed.
Digest does not handle missing files or directories well. So safe_digest
was written to wrap digest and catch errors.

In b1cb9e8 it was discovered that
the error catching was too optimistic and was eating errors that should
be reported to the user. To fix this, logic was added to catch
the known errors from digest and re-raise all other errors.

The only known error at that time was if the file did not exist.
Unfortunately digest also throws an error if it is passed a directory.
With the introduction of the new testthat tests folder structure,
all tests folders also include a subfolder called testthat. When
digest was passed the folder name it was failing.

Rather than filtering for folder, I've expanded the known errors logic.
The list of known errors is now encapsulated in a known_errors function.
An error is checked against this list, and if it isn't found then the
error is re-raised.
@KentonWhite
Copy link
Author

Added a bullet point to News. Travis is failing with the warning

Warning: /tmp/RtmpDScBVE/Rbuild1b14f561f1f/testthat/man/testthat.Rd:26: unknown macro '\em'

Don't think this change has caused that!

@hadley
Copy link
Member

hadley commented Sep 17, 2014

Can you please rebase/merge against master?

@hadley hadley closed this in e3353f7 Sep 17, 2014
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.

None yet

2 participants