CI: diff json mismatches

[ynfle]

This introduces line based diffing showing the diff (Git default styled) between the pretty printed output of the results.json & expected_results.json using the diff package

Previous method (link to workflow run)

Current Method (link to line in workflow run)

There are still the following issue which have to be ironed out:

• [ ] Installing the diff package on the Dockerfile. I currently fails (see here)
• [x] The diff package has a style error. See failed run here. This is why we have the extra commit of changing to --styleCheck:usages just to show the diff of adding this diffing functionality (😉 pun intended)
  • MY PR to update the was styling was merged

[ynfle]

@ErikSchierboom Any ideas how to install the package on the docker file only when testing in the CI without maintaining a whole new Dockerfile?

[ErikSchierboom]

@ynfle Not really. Is it a big dependency? Otherwise, I'd probably just bake it into the Dockerfile.

[ynfle]

@ynfle Not really. Is it a big dependency? Otherwise, I'd probably just bake it into the Dockerfile.

Doesn't seem like it. 279 lines of nim with out any external dependencies. Let me see. I may be able to add it to the entrypoint

[ynfle]

That didn't work because nimble isn't copied over from the base image

[ee7]

We'd be adding to the production Dockerfile just to get nicer output on failing tests at CI-time, right? I'd prefer to avoid that - it's best practice for a Dockerfile to contain the bare minimum. Can we do it another way?

For example, what do you think about a run-time condition that calls the diff executable (or git diff) if we aren't inside the image? A possible way to determine "are we in the image?" is to check if the executable exists - I think diff isn't in the image, and git isn't either. Sure, calling an executable has a huge relative overhead compare to calling a Nim proc, but this isn't performance-sensitive, and it only happens in CI, and only for failing tests. Otherwise, a compile-time condition somehow: maybe a when defined or similar for producing pretty diffs only when we're not running tests inside the image?

If we really need to bake in the diffs package, and this is a distant runner-up for me: I'd suggest that we need to pin its commit ref. The current PR uses the latest tag of the diffs package, which means that someone with write access to it can push a buggy/malicious update that compromises the image, possibly in a way that is not detected at build-time. Pinning to a tag is weaker, as someone can tag a different commit with the same tag.

The Nimble that's distributed with Nim doesn't support lockfiles yet, so working with commit-ref-pinned package versions is painful, and can cause unexpected behavior. I did intend to eventually add some Nimble packages to the image, but I think that most users won't import them. Nimble isn't in the image right now because we weren't using it, and because Nimble-only vulnerabilities are possible (e.g. https://nvd.nist.gov/vuln/detail/CVE-2021-21373).

[ynfle]

I disagree that it isn't of value. I have spent a while debugging the json diff to see if the changes are expected or not before writing and committing them. Yes, the two string outputs could technically be saved as files and diffed locally, but it's considerably more apparent and quicker to see it in the output on the CI. This is also useful for people that don't (or can't) have docker installed to test inside the docker container.

What I decided to do is to checkout the repo at the desired commit and then move the file in to the container

[ynfle]

This seems to work. There is another style usage that has to be fixed, so for now, the diff repo is my fork

[ynfle]

Cleaned up.

No adding anything to the the Dockerfile.

[ErikSchierboom]

Interesting approach! 👍

[ynfle]

Sorry, your comment came through after I merged. Happy to address in follow up PR

[ee7]

Yeah, no worries.

[ynfle]

I disagree that it isn't of value.

Sorry, I expressed myself poorly. I'm often in a hurry when writing. I actually do think that this has value, especially when upgrading Nim versions, when the tests fail due to line numbers changing in error messages from unittest.nim.

Totally fine

What I decided to do is to checkout the repo at the desired commit and then move the file in to the container

This approach is okay by me, but it does mean that a plain git clone of this repo and running nim r tests/trunner.nim will fail, because the user probably doesn't have the diff package installed.

Yes. I think that we should've used nimble and then could nimble test be run which would install the necessary packages when run locally.