Skip to content

Introduce strategies for test ID generation#1306

Merged
Evangelink merged 12 commits intomainfrom
dev/haplois/id-gen
Oct 27, 2022
Merged

Introduce strategies for test ID generation#1306
Evangelink merged 12 commits intomainfrom
dev/haplois/id-gen

Conversation

@Evangelink
Copy link
Copy Markdown
Member

@Evangelink Evangelink commented Oct 3, 2022

Fixes #1286 and fixes AB#1634557

TODO:

  • Add comments
  • Add tests

@Haplois Haplois force-pushed the dev/haplois/id-gen branch from 2f839f9 to 1e0ba6a Compare October 5, 2022 10:08
@Haplois
Copy link
Copy Markdown
Contributor

Haplois commented Oct 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Contributor

@Haplois Haplois left a comment

Choose a reason for hiding this comment

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

How is this going? Would you like any help @Evangelink?

@Evangelink
Copy link
Copy Markdown
Member Author

Hey thanks for asking @Haplois. Looks like I just need to fix CI tests (they are passing locally for me). There are still some tickets open (not fixed by this change) that I need to investigate but I feel confident we will be able to merge this one soon(TM).

@Evangelink
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

@Haplois please feel free to do a review when you have some free time.

Haplois
Haplois previously approved these changes Oct 26, 2022
Copy link
Copy Markdown
Contributor

@Haplois Haplois left a comment

Choose a reason for hiding this comment

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

I think removing bail-out strategy might cause issues for some users, we need to rethink that, or at lease do a version increment. My approval is grey now :(

@Evangelink
Copy link
Copy Markdown
Member Author

Thanks for the review guys!

@Haplois What do you mean by the bail-out strategy?

@Haplois
Copy link
Copy Markdown
Contributor

Haplois commented Oct 26, 2022

See my comment above on the code.

@Evangelink Evangelink dismissed stale reviews from Haplois and MarcoRossignoli via 6896179 October 26, 2022 13:09
@Evangelink
Copy link
Copy Markdown
Member Author

@MarcoRossignoli @Haplois I have addressed all comments, if you want to have another look. @Haplois There is one open question remaining for you.

@Haplois
Copy link
Copy Markdown
Contributor

Haplois commented Oct 26, 2022

@Evangelink uploaded a patch file for the broken ID gen. Can't push to this branch anymore.

@Evangelink
Copy link
Copy Markdown
Member Author

@Haplois I have applied and updated your patch. Let me know if you have any other comment or if I can move forward with merging the PR.

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.

Introduce strategies for test ID generation

3 participants