Skip to content

Updated readInt method so streams are closed on failure or success #1476

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

Open
srnkan wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Updated readInt method so streams are closed on failure or success #1476

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2dbe7b3
Updated readInt so streams are closed
srnkan Mar 11, 2026
b9b794e
made tests for UpdateCheckTest.java and updated necessary files
srnkan Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/src/processing/app/UpdateCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,12 @@ protected boolean promptToOpenContributionManager() {

protected int readInt(String filename) throws IOException {
URL url = new URL(filename);
InputStream stream = url.openStream();
// try-with-resources auto closes things of type "Closeable" even the code throws an error
try(InputStream stream = url.openStream();
InputStreamReader isr = new InputStreamReader(stream);
BufferedReader reader = new BufferedReader(isr);
return Integer.parseInt(reader.readLine());
BufferedReader reader = new BufferedReader(isr)) {
return Integer.parseInt(reader.readLine());
}
Comment on lines +211 to +215
Copy link
Copy Markdown
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

this is really cool! good use of try-with-resources.

Copy link
Copy Markdown
Author

@srnkan srnkan Mar 23, 2026

Choose a reason for hiding this comment

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

I added tests to the UpdateCheckTest.java file, with 16 tests. Also, Mark and I noticed that in the build.gradle.kts file under the app build, only kotlin tests are ran. So, I modified that file to include java tests.

}


Expand Down
4 changes: 4 additions & 0 deletions app/test/processing/app/UpdateCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package processing.app;

public class UpdateCheckTest {
}
Copy link
Copy Markdown
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

This could use some tests!

Copy link
Copy Markdown
Author

@srnkan srnkan Mar 11, 2026

Choose a reason for hiding this comment

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

I agree, but I was unable to compile tests for the issue because of the ongoing testing issue. Once that issue is resolved, I'm happy to go back and add tests. Thank you!

Copy link
Copy Markdown
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

OH my gosh, yes. I just want to clarify my understanding of the testing issue, is it the broken tests for ProcessingPlugin ? Or something in addition to that?

Copy link
Copy Markdown

@Ebaron96 Ebaron96 Mar 11, 2026

Choose a reason for hiding this comment

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

looks good to me as well!

Copy link
Copy Markdown
Author

@srnkan srnkan Mar 11, 2026

Choose a reason for hiding this comment

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

It's the same issue that Mark was talking about in the discord yesterday. Certain .gradlew tests fail automatically

Copy link
Copy Markdown
Collaborator

@catilac catilac Mar 11, 2026

Choose a reason for hiding this comment

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

ohh yes, thanks for clarifying @srnkan! that isn't a blocker in this case. we'll just assume those tests are broken, so feel free to get started on this test. it could be helpful to toss in a little assertTrue(true) as a sanity check