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

tests for #135 #241

Closed

Conversation

andyleiserson
Copy link
Collaborator

Some tests I wrote for #135. Haven't settled on a strategy yet.

}

#[tokio::test]
pub async fn narrow() -> Result<(), BoxError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems that you don't need those tests to be async?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried changing them to regular tests, but there is a call to tokio::spawn within InMemoryNetwork::new within make_world.

// // do something *without* narrowing context by `i` or using it as RecordId
// }
// ```
#[ignore] // broken, see #135
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the material difference is between this test and the previous one. What am I missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that, in the specific case where i is a record ID, narrowing a context twice in this way should not be an error. It is okay to construct the same context path multiple times, as long as you're doing it for different record IDs.

In the test case it would be possible to avoid the issue by moving the call to narrow outside the loop, but in general the body of the for record_id loop can be spread across multiple functions and it may not be desirable to pull the calls to narrow all the way up. (main...andyleiserson:ipa:issue135 shows what that might look like.)

@andyleiserson
Copy link
Collaborator Author

No longer applicable after #253.

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