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

[DRAFT] Feature/retestid without gm #563

Closed
wants to merge 2 commits into from

Conversation

roesslerj
Copy link
Contributor

@roesslerj roesslerj commented Mar 13, 2020

  • the code follows the Clean Code guidelines.
  • the necessary tests are either created or updated.
  • all status checks (Travis CI, SonarCloud, etc.) pass.
  • your commit history is cleaned up.
  • you updated the changelog.
  • you updated necessary documentation within retest/docs.

Since retestId is now created deterministically, we can reference it before we even have a Golden Master. This is gonna be helpful in a variety of situations, but especially when generating Surili tests.

@sonarcloud
Copy link

sonarcloud bot commented Mar 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

40.0% 40.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@beatngu13 beatngu13 left a comment

Choose a reason for hiding this comment

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

Having a deterministic RetestIdProvider is not a contract to my knowledge, so we cannot rely on this. For example, we still ship RandomSuffixRetestIdProvider and the user is free to provide a custom implementation. Then this doesn't work.

If we want RetestIdProviders to be deterministic in general, we have to document and require this contract (like e.g. equals requires an equivalence relation), otherwise there will be undefined behavior.

Also, what happens if slight state changes lead to different counts? We no longer lookup the element in the old state and do a 1-on-1 assignment, instead we select it solely on the basis of the retest ID I guess?

It would rather mark this as a draft. We have to discuss the determinism and we need proper tests, at least for the aforementioned situations. If we do such a "drastic" change without tests, and no existing tests are failing, we are doing something wrong.

@roesslerj roesslerj closed this Mar 18, 2020
@roesslerj roesslerj reopened this Mar 18, 2020
@beatngu13 beatngu13 changed the title Feature/retestid without gm [DRAFT] Feature/retestid without gm Mar 18, 2020
@beatngu13
Copy link
Contributor

Off-topic: How to convert an existing PR into a draft? 🤷‍♂️

@diba1013
Copy link

diba1013 commented Mar 18, 2020

Off-topic: How to convert an existing PR into a draft? 🤷‍♂️

This is not possible as far as I am aware: isaacs/github#1547

@beatngu13
Copy link
Contributor

Closed until someone has time to address the open issues as part of #570.

@beatngu13 beatngu13 closed this Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants