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 test info mod rs #827

Merged
merged 29 commits into from
Oct 29, 2022
Merged

Add test info mod rs #827

merged 29 commits into from
Oct 29, 2022

Conversation

atluft
Copy link
Contributor

@atluft atluft commented Oct 17, 2022

Rebased PR #822
Didn't realize that would close the other PR, sorry.

src/info/mod.rs Outdated Show resolved Hide resolved
src/info/mod.rs Outdated Show resolved Hide resolved
@atluft
Copy link
Contributor Author

atluft commented Oct 19, 2022

Trying a new approach in 3e2757d
Test creates repo and expected result using shell script via git-testttools
Test reads expected result from file created by shell script
Test perform action
Test compare actual to expected

Thinking about how to handle git version (like excluding it) and removing colors.

@spenserblack
Copy link
Collaborator

These assertion diffs are pretty hard to read 😆
I've used pretty_assertions before to get better diffs.

@atluft
Copy link
Contributor Author

atluft commented Oct 20, 2022

Thank you @spenserblack , here is a pretty assertion:

image

@atluft
Copy link
Contributor Author

atluft commented Oct 20, 2022

@spenserblack and @o2sh
Really like the output of pretty_assert that is very easy to see the issues.

What are thoughts on using regular expressions in the expected output?
I have the wrong regex for REGEX_ANYTHING, ".*" is only good for end of lines.
Wanted a quick review and collect ideas before putting too much effort into this.

Would this be better if a phrases were used: MATCH_UNTIL_END_OF_LINE or MATCH_WORD ?

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Those diffs look much better now, thank you!

tests/fixtures/language_repo.sh Outdated Show resolved Hide resolved
src/info/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Testing against a regex, and stripping out the ANSI escape sequences, looks much better IMO! 👍
Almost there! Just a few more comments.

src/info/mod.rs Outdated Show resolved Hide resolved
src/info/mod.rs Outdated Show resolved Hide resolved
src/info/mod.rs Outdated Show resolved Hide resolved
atluft and others added 2 commits October 21, 2022 18:49
Co-authored-by: Spenser Black <spenserblack01@gmail.com>
Co-authored-by: Spenser Black <spenserblack01@gmail.com>
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Now I want to write a macro to better support snapshot testing 😆

src/info/mod.rs Outdated Show resolved Hide resolved
src/info/mod.rs Outdated Show resolved Hide resolved
@spenserblack
Copy link
Collaborator

Here I was about to make a snapshot testing library, and in my research found that one exists 😆
https://crates.io/crates/insta

It might be worth using this library for the JSON output test.

@atluft
Copy link
Contributor Author

atluft commented Oct 26, 2022

Have a look at 6486d50

Ugh, then have a look at 75b427c for the insta snapshot file. The .gitignore file has *.snap in it to ignore snapshots.

Insta setup went like this:

  cargo add --dev insta --features json,redaction,redactions
  cargo install cargo-insta
# add assert_json_snapshot to code
  cargo test -- --nocapture
  cargo insta review

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Great work!

The .gitignore file has *.snap in it to ignore snapshots.

We might want to adjust the .gitignore so that it ignores only the files it should ignore.

@o2sh Do you remember why *.snap is in the .gitignore?

src/info/mod.rs Outdated Show resolved Hide resolved
@o2sh
Copy link
Owner

o2sh commented Oct 26, 2022

@o2sh Do you remember why *.snap is in the .gitignore?

If I remember correctly, this line was added for when I used to build the snapcraft package locally before publishing it to the Snap store - which is now done automatically via a Github link.

I think this line can be safely removed.

src/info/mod.rs Outdated Show resolved Hide resolved
@atluft
Copy link
Contributor Author

atluft commented Oct 28, 2022

Change 69c6229 caused a snapshot change, testing gitignore change ;-)

Also, gave another chance to use cargo insta review that is so cool how it "fixes" up the snapshot file for the test.

@spenserblack
Copy link
Collaborator

that is so cool how it "fixes" up the snapshot file for the test.

Yeah, snapshot testing is pretty interesting. It leans a bit towards "acceptance". That is, instead of a human writing a test for the machine to run and the code to pass, a human reviews the output and tells the machine if they like it.

@o2sh o2sh merged commit a1f1987 into o2sh:main Oct 29, 2022
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

3 participants