kubernetes-sigs / structured-merge-diff

Test cases and implementation for "server-side apply"
Apache License 2.0
105 stars 62 forks source link

Add test to show that typename and version don't always match #219

Closed apelisse closed 2 years ago

apelisse commented 2 years ago

The test is flaky since the behavior depends on the order of which the versions are processed, and the current code that prevents that from happening depends on an invalid name for the version.

To make sure you get the failure:

$ go test -run=TestVersionDoesntMatchTypename -count=10 ./merge/
apelisse commented 2 years ago

The bot failed, that's a good thing, it's supposed to :-)

apelisse commented 2 years ago

Unfortunately I couldn't really make the test non-flaky, maybe I should run it in a loop internally so it fails more often ... suggestions?

lavalamp commented 2 years ago

Yeah you could internally do 10 tries or something. Or you could fix the code 😅

apelisse commented 2 years ago

Yeah you could internally do 10 tries or something. Or you could fix the code 😅

I'll just do both ... :-)

apelisse commented 2 years ago

I've run the test a 100 times and it didn't flake!

zqzten commented 2 years ago

I've been busy this week, sorry😥. Thanks for fixing it for me, looks good!

k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, zqzten

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/OWNERS)~~ [apelisse] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
zqzten commented 2 years ago

@apelisse Apologize for bothering, I'm wondering why did this PR get closed? Is the version fix no longer needed?

apelisse commented 2 years ago

Wasn't this merged into another PR? I'll try to take a look.

zqzten commented 2 years ago

Nope, at least the fix is not in the upstream code so far.

apelisse commented 2 years ago

OK, this was a clean rebase on top of master, so none of this was indeed merged. Thanks!