Skip to content

test: simple primitives #4

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
Merged
merged 11 commits into from
Aug 7, 2022
Merged

test: simple primitives #4

merged 11 commits into from
Aug 7, 2022

Conversation

@BFergerson
Copy link
Collaborator

@BFergerson BFergerson commented Aug 7, 2022

No description provided.

@BFergerson
Copy link
Collaborator Author

BFergerson commented Aug 7, 2022

@MrMineO5, modeled the new tests after the design of SimplePrimitivesLiveInstrumentTest.

I had some trouble implementing it with OOP so ended up using a utility class.
Any thoughts on how to improve the design?

@MrMineO5
Copy link
Contributor

MrMineO5 commented Aug 7, 2022 •
edited
Loading

Why are you returning a function that returns the file name in TestUtils.getFilename?
Also, wouldn't that regular expression break with spaces in the file path?

@BFergerson
Copy link
Collaborator Author

BFergerson commented Aug 7, 2022

Something I copied from the JVM implementation. I wanted the source and line numbers of the live instruments to be dynamic so you can move the files around and modify them without breaking them. Adding TestUtils.getFilename should allow you to rename that file and the test will still pass.

@BFergerson
Copy link
Collaborator Author

BFergerson commented Aug 7, 2022

@MrMineO5, thanks for the improvments. I like how these tests are coming out. Looks very easy to maintain.

@BFergerson BFergerson marked this pull request as ready for review August 7, 2022 18:19
@BFergerson BFergerson merged commit c747374 into master Aug 7, 2022
@BFergerson BFergerson deleted the self-test-design branch August 7, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

@BFergerson @MrMineO5