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.

Sign up for GitHub

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

Jump to bottom

test: add cases for format_location #541

Merged
merged 5 commits into from
Apr 18, 2024
Merged

test: add cases for format_location #541

merged 5 commits into from
Apr 18, 2024

Conversation

Rustin170506
Copy link
Collaborator

Because this function uses a regex, it's better to add some tests to make sure we don't break anything if we want to change the regex.

@Rustin170506 Rustin170506 requested a review from a team as a code owner April 9, 2024 11:11
@Rustin170506 Rustin170506 requested a review from hds April 9, 2024 11:11
hds
hds requested changes Apr 9, 2024
View reviewed changes
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

This is a great idea, I think it makes sense to test paths from other platform too.

tokio-console/src/state/mod.rs Outdated Show resolved Hide resolved
@Rustin170506
test: add cases for format_location
b124699 
Because this function uses a regex,
it's better to add some tests to make sure
we don't break anything if we want to change the regex.
@Rustin170506
test: add macOS case
2950042
@Rustin170506 Rustin170506 requested a review from hds April 15, 2024 11:42
hds
hds approved these changes Apr 15, 2024
View reviewed changes
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

hawkw
hawkw requested changes Apr 15, 2024
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

if possible, i would like to remove the conditional compilation from these tests. besides that, this is great! thank you!

tokio-console/src/state/mod.rs Outdated Show resolved Hide resolved
tokio-console/src/state/mod.rs Outdated Show resolved Hide resolved
Rustin170506
Rustin170506 commented Apr 17, 2024
View reviewed changes
Copy link
Collaborator Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@Rustin170506 Rustin170506 requested a review from hawkw April 17, 2024 12:20
hawkw
hawkw approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

one last suggestion, although it's not a blocker — i'm fine with the current code, but i think separating the tests might be a little nicer.

tokio-console/src/state/mod.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 enabled auto-merge (squash) April 18, 2024 00:41
@Rustin170506 Rustin170506 merged commit 4d91f5d into tokio-rs:main Apr 18, 2024
17 checks passed
@Rustin170506 Rustin170506 mentioned this pull request May 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@hds hds hds approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rustin170506 @hds @hawkw