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

test(compare-command): Add a functional test for the text diff method #7950

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

sschuberth
Copy link
Member

No description provided.

@sschuberth sschuberth requested a review from a team as a code owner November 28, 2023 19:52
@sschuberth sschuberth enabled auto-merge (rebase) November 28, 2023 19:53

class CompareCommandFunTest : WordSpec({
"The text diff method" should {
"ignore time and environment differences in an analyzer result" {
Copy link
Member

Choose a reason for hiding this comment

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

Looking at what this tests covers vs. the test assets, I believe the assets contain a lot of unneeded stuff.

I believe it'd be better if the test only had one result hard-coded, then it should make a copy with environment modified and then assert the diff is empty.

Doing so could re-use an ORT file coded in Kotlin we already have, maybe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not sure about this. I deliberately created real-life analyzer results, once from a Jenkins run on Linux, once from a manual run on Windows, to ensure the command works for these, instead of coming up with some artificial results that I'm patching up, with having in mind how the implementations works.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, would you be ok with sticking to the current approach @fviernau?

Copy link
Member

Choose a reason for hiding this comment

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

Not really convinced, but the maybe unnecessary maintainance cost I tried to avoid are isolated and limited. So, ok let's make this compromise.

@sschuberth sschuberth requested review from a team and fviernau November 29, 2023 09:40
@fviernau fviernau dismissed their stale review November 29, 2023 11:15

Not 100% about above either

@sschuberth sschuberth merged commit 6eeb729 into main Nov 30, 2023
20 checks passed
@sschuberth sschuberth deleted the compare-command-funtest branch November 30, 2023 08:02
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