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

This PR fixes the issue of flush() #1358

Merged
merged 9 commits into from
Jul 25, 2023
Merged

This PR fixes the issue of flush() #1358

merged 9 commits into from
Jul 25, 2023

Conversation

nmondal
Copy link
Contributor

@nmondal nmondal commented Jul 22, 2023

As mentioned here.
#1356

@rbri
Copy link
Collaborator

rbri commented Jul 23, 2023

i guess spotless will not be happy with this but outside of that i'm fine with the change

lets see what the ci has to say

@nmondal
Copy link
Contributor Author

nmondal commented Jul 23, 2023

@rbri can you help me out here? What I need to do to make the spotless thing passed?

@rbri
Copy link
Collaborator

rbri commented Jul 23, 2023

@nmondal simply run ./gradlew spotlessApply

@nmondal
Copy link
Contributor Author

nmondal commented Jul 23, 2023

@nmondal simply run ./gradlew spotlessApply

Done that, figured that one out, was a bit tricky Java 8 and all.. but done now.
Should the CI CD run automatically again or...?

@rbri
Copy link
Collaborator

rbri commented Jul 23, 2023

@nmondal no i have to approve the ci run....

@nmondal
Copy link
Contributor Author

nmondal commented Jul 23, 2023

@rbri we have lift-off endorsements!
What are the next steps?

@rbri
Copy link
Collaborator

rbri commented Jul 23, 2023

  1. be proud ;-)
  2. wait for the committers to pick up this
  3. remind the committer to pick up this - go back to step 2.

@@ -31,8 +31,24 @@ public void setup() {
}

@Test
public void printStdout() throws ScriptException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nmondal can you please bring back to old test and add you as a new one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rbri the old test was not testing anything.
If the old test was "correct" we would never have had the bug in the first place.
Being said that, if you still wish we can. But do we want to?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the test at least makes sure the print function is supported and works without an exception.
And it does not harm to have one more test (and it is good for the statistics :-))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Will do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the test at least makes sure the print function is supported and works without an exception. And it does not harm to have one more test (and it is good for the statistics :-))

Done. Done. Done.
🕺🏽

@rbri can you please do the honors of running the pipeline?

@rbri rbri merged commit 76fc05e into mozilla:master Jul 25, 2023
3 checks passed
@rbri
Copy link
Collaborator

rbri commented Jul 25, 2023

@nmondal many thanks for your 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.

None yet

2 participants