Skip to content

Conversation

@shs96c
Copy link
Collaborator

@shs96c shs96c commented Feb 28, 2025

It turns out that making minor changes in the test data is prohibitively complicated since the tests very much rely on the git shasums, and any change will need to be reflected here. We also can't rebase the changes since that would break historical builds.

Rather than rely on an external repo, instead, we now create the data within our own build. The individual states we want to use are modelled as separate directories within the integration test's testdata dir, and each state is given a meaningful name.

@shs96c shs96c marked this pull request as ready for review March 7, 2025 12:51
shs96c added 6 commits March 7, 2025 15:12
It turns out that making minor changes in the test data is prohibitively
complicated since the tests very much rely on the git shasums, and any
change will need to be reflected here. We also can't rebase the changes
since that would break historical builds.

Rather than rely on an external repo, instead, we now create the data
within our own build. The individual states we want to use are modelled
as separate directories within the integration test's `testdata` dir,
and each state is given a meaningful name.
@shs96c shs96c force-pushed the build-test-data branch from 01b0508 to 7ff8db4 Compare March 7, 2025 15:26
@shs96c shs96c force-pushed the build-test-data branch from b50c9ee to 003205c Compare March 7, 2025 16:56
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Totally understand and agree with the problem (and sorry for not having automated the repo creation in the first place, it would've been polite/useful!)

I do think there are a couple of draw-backs to this approach, but I think with a pretty small amount of extra work we can get the best of both worlds?

The two big drawbacks to me are it makes it hard to verify the tests are actually testing what we think (if there are bugs in the generation, we'll never notice, and it makes it hard to manually run a test (e.g. right now I can just clone the test repo and run a manually built TD against it).

But given you're already generating git repos here, my ideal would be that we make a main function we can run which just generates a bunch of tags in a git repo with the states we care about (maybe named after the directory names + date of timestamp of generation or something), and generates the Commits class that contains the constants with the commit shas.

Which I think is roughly a trivial amount of work to do on top of what you're already doing (just "save and push" the commits, rather than "generate and discard them"), but gives us the best of both worlds?

WDYT?

@shs96c
Copy link
Collaborator Author

shs96c commented Mar 7, 2025

Would you like to have a go at this once this PR lands? Or send a patch on top of this PR?

@illicitonion
Copy link
Collaborator

Sure, I can give it a go, but likely won't be for at least a few days :)

@darkrift darkrift mentioned this pull request Mar 24, 2025
@sitaktif
Copy link
Collaborator

@illicitonion do you suggest we keep a separate testdata repo, or is the idea that the generator creates the git repo ~on the fly before testing and passes it as test data somehow?

illicitonion pushed a commit that referenced this pull request May 1, 2025
This canonicalize all the labels in all attributes found that are label related using the output of `bazel mod dump_repo_mapping ""` for each commit.

This addresses #105

It doesn't have any integration tests yet because it would be much better to leverage #104 which is still in the works but it was tested against our internal monorepo and the reproducer in #105 

Label like attributes (string defined but has nodep = True) are special and handled as an exception as if they were labels, and thus also converted. (See [this thread](https://bazelbuild.slack.com/archives/CDCMRLS23/p1742821059464199))
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.

3 participants