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.

Sign up for GitHub

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

Jump to bottom

Ignore spaces at the ends of lines in the MetaTest StringReplacementEvaluator, fixes #156 #233

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Ignore spaces at the ends of lines in the MetaTest StringReplacementEvaluator, fixes #156 #233

merged 7 commits into from
Jul 27, 2023

Conversation

Una865
Copy link
Contributor

@Una865 Una865 commented Jul 7, 2023 •
edited by martinmladenov
Loading

Added regex expression for capturing spaces on the end of the line.

Fixes #156

mauricioaniche
mauricioaniche reviewed Jul 7, 2023
View reviewed changes
Copy link
Contributor

@mauricioaniche mauricioaniche left a comment

Choose a reason for hiding this comment

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

Thanks for the MR! @martinmladenov the approval is yours!

Question: do we need to update the README too?

@mauricioaniche
Copy link
Contributor

Just rebased and put the tests to run!

@martinmladenov this one is for you to merge once you feel it's ready!

@mauricioaniche
Copy link
Contributor

Oh, tests are breaking?

@mauricioaniche mauricioaniche force-pushed the ign-endSpaces-stringReplacement branch from a266efb to 418cdfd Compare July 17, 2023 16:03
@mauricioaniche
Copy link
Contributor

Hey, @Una865, it seems like the tests are breaking. I just re-ran the pipeline with the most recent version of the code. Would you mind taking a look?

@mauricioaniche mauricioaniche changed the title fix #156 Ignore spaces at the ends of lines in the MetaTest StringReplacementEvaluator, fixes #156 Jul 19, 2023
@martinmladenov
Copy link
Collaborator

Using \s in both sides of the regex caused blank lines to be removed by merging the previous and the next lines (e.g. 123\n\n456 would be turned into 123456). This was causing an exception when running meta tests, and meta tests would be mistakenly marked as passing when they shouldn't be due to #256. (However, this still doesn't explain why having this error in meta tests doesn't trigger a generic failure in the meta test sub-result but simply causes individual test cases to fail...)

mauricioaniche
mauricioaniche approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@mauricioaniche mauricioaniche left a comment

Choose a reason for hiding this comment

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

If you trust this regex, I trust on it too, @martinmladenov !

@martinmladenov
Copy link
Collaborator

I trust our tests and according to them, this regex works! 😆

@martinmladenov martinmladenov merged commit 5729e9a into SERG-Delft:main Jul 27, 2023
8 checks passed
@mauricioaniche
Copy link
Contributor

That's the answer I was looking for 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinmladenov martinmladenov martinmladenov approved these changes

@mauricioaniche mauricioaniche mauricioaniche approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ignore spaces at the ends of lines in the MetaTest StringReplacementEvaluator
3 participants
@Una865 @mauricioaniche @martinmladenov