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

Fix RerunLastFailures problem. Appear the perfix only one time #1512

Closed
wants to merge 2 commits into from

Conversation

Eqerem-Hena
Copy link
Contributor

Fix the #1342 issue.

@fhoeben
Copy link
Collaborator

fhoeben commented Jun 6, 2024

@Eqerem-Hena Thanks a lot for your PR!

I believe you are generating unique names for each re-rerun by adding a counter at the end, correct?
To me this seems overkill as it keeps making a lot of pages if the rerun is retried often.

Like I stated in the issue just having one rerun per page/suite seems fine. So this PR could be much simpler: just don't add the RerunLastFailures_ prefix if the full page name already starts with that.
Do you think that is too limited?

@fhoeben
Copy link
Collaborator

fhoeben commented Jun 6, 2024

It also seems like some of the existing tests failed, can that be related to your change or did we let something else slip through and break that already?

@Eqerem-Hena
Copy link
Contributor Author

@Eqerem-Hena Thanks a lot for your PR!

I believe you are generating unique names for each re-rerun by adding a counter at the end, correct? To me this seems overkill as it keeps making a lot of pages if the rerun is retried often.

Like I stated in the issue just having one rerun per page/suite seems fine. So this PR could be much simpler: just don't add the RerunLastFailures_ prefix if the full page name already starts with that. Do you think that is too limited?

I think it is a better idea to add the counter at the end, as in this way it will be more understandable to the user which rerun page is.

@Eqerem-Hena
Copy link
Contributor Author

It also seems like some of the existing tests failed, can that be related to your change or did we let something else slip through and break that already?

I don't think so. It's probably from a previous commit.

@fhoeben
Copy link
Collaborator

fhoeben commented Jun 7, 2024

The original submitter of the issue indicated:

This would help keep my FitNesseRoot directory from exploding...

So they prefer not to have an additional file for each rerun.

@Eqerem-Hena
Copy link
Contributor Author

The original submitter of the issue indicated:

This would help keep my FitNesseRoot directory from exploding...

So they prefer not to have an additional file for each rerun.

I did a new commit. Please, check it out!

Copy link
Collaborator

@fhoeben fhoeben left a comment

Choose a reason for hiding this comment

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

Looks really good. Thank you. Some minor comments and then we can merge

@@ -295,7 +295,12 @@ protected String getRerunPageName() {
PageCrawler pageCrawler = page.getPageCrawler();
WikiPagePath fullPath = pageCrawler.getFullPath();
String fullPathName = PathParser.render(fullPath);
return "RerunLastFailures_"+fullPathName.replace(".","-");
if (fullPathName.startsWith("RerunLastFailures_")) {
String newFullPathName = fullPathName.replace(".", "-");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes more sense to to the replace on line 297 to remove the duplicated code from both branches

@fhoeben
Copy link
Collaborator

fhoeben commented Jun 16, 2024

Could you also update the release notes?

@fhoeben
Copy link
Collaborator

fhoeben commented Jul 7, 2024

Cleaned up the tests in #1519

@fhoeben fhoeben closed this Jul 7, 2024
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.

2 participants