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

Add fetch tests #136

Closed
wants to merge 2 commits into from
Closed

Add fetch tests #136

wants to merge 2 commits into from

Conversation

JONBRWN
Copy link

@JONBRWN JONBRWN commented Jun 9, 2020

  • extracts test helpers (OpsRecorder, etc) from the cmd/update tests for usage with other cmds
  • adds unit tests for cmd/fetch for the following scenarios:
    • empty-file
    • empty-deps
    • self-referential
    • mutually-recursive

JONBRWN added 2 commits June 8, 2020 22:16
  - adds `fetch` tests for the following scenarios:
    * empty deps
    * empty dir
    * mutually recursive
    * self referential
@JONBRWN JONBRWN requested a review from SeanTAllen June 9, 2020 15:38
test(_TestFetchEmptyDeps)
test(_TestFetchEmptyFile)
test(_TestFetchMutuallyRecursive)
test(_TestFetchSelfReferential)
Copy link
Member

Choose a reason for hiding this comment

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

I think you should add a test for transitive dependencies. That would mean renaming the regression-120 stuff to be a more generic "transitive dependencies" test.

Copy link
Author

Choose a reason for hiding this comment

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

will do

Verify that when using corral.json with empty deps, that there
are never any sync, tag query, or checkout operations executed.
"""

Copy link
Member

Choose a reason for hiding this comment

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

extra newline here and elsewhere in this file.


fun val checkout_op(rev: String, next: RepoOperation): RepoOperation =>
_RecordedCheckout(_recorder, next)

Copy link
Member

Choose a reason for hiding this comment

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

extra newline


primitive _TestRepoCache
fun apply(auth: AmbientAuth, path: String): FilePath ? =>
FilePath(auth, path)?
Copy link
Member

Choose a reason for hiding this comment

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

needs a newline at the end

@@ -0,0 +1,101 @@
use "files"
Copy link
Member

Choose a reason for hiding this comment

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

rather than sticking everything in "_test_helpers.pony"

what about a file for the helpers that are only opsrecorder test related?

_test_helpers is very generic and as bad as a "util" package.

so it could be

"_test_data_test_helpers"
"_ops_recorder_test_helpers"

or perhaps better:

_test_helpers_test_data
_test_helpers_ops_recorder

I'm not sure what is best, but I think that we can do better than _test_helpers.

Copy link
Author

Choose a reason for hiding this comment

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

I'll break this out in a meaningful way

@SeanTAllen
Copy link
Member

A few minor comments left.

I think the big one is that there is no test for transitive dependencies being handled correctly, as I noted.

Base automatically changed from master to main February 8, 2021 18:42
@SeanTAllen SeanTAllen closed this Feb 24, 2024
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