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

Fixes TipImageTests in org.eclipse.tips.tests #525 #7

Closed
wants to merge 1 commit into from

Conversation

Michael5601
Copy link
Collaborator

The content of the 4 disabled tests was very random before the change. There are 3 possibilities to deal with those tests:

  1. Just delete the tests completely (which feels best for me)
  2. Only test if setExtension has really set the extension by calling getExtension afterwards. (is very trivial and works only if the URL constructor was used because of the bad implementation of getExtension)
  3. Use the solution presented in this PR (my problem with this solution is that it changes the too much of the old behavior and especially that the fields fURL and fBase64Image cannot be final anymore)

Can you please give me feedback to this?

Copy link

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

@Michael5601 I'd propose you submit this proposal with suggested changes. Please write a commit message that is focussed on correcting setExtension of the TipImage rather than on fixing test, because that's what the PR will actually be about. Changes will require fBase64Image to not be final anymore, but fURL can remain final.

Comment on lines 215 to 227
if (isURLSet()) {
String oldExtension = getExtension();
try {
fURL = new URL(fURL.toString().replace(oldExtension, newExtension));
} catch (MalformedURLException e) {
e.printStackTrace();
}
} else {
int from = fBase64Image.indexOf('/') + 1;
int to = fBase64Image.indexOf(';');
String oldExtension = fBase64Image.substring(from, to).trim();
fBase64Image = fBase64Image.replace(oldExtension, newExtension);
}

Choose a reason for hiding this comment

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

Since there is no specification saying that changing the extension shall actually change the URL but only the tag in the image string, I'd propose to simply change this as follows:

Suggested change
if (isURLSet()) {
String oldExtension = getExtension();
try {
fURL = new URL(fURL.toString().replace(oldExtension, newExtension));
} catch (MalformedURLException e) {
e.printStackTrace();
}
} else {
int from = fBase64Image.indexOf('/') + 1;
int to = fBase64Image.indexOf(';');
String oldExtension = fBase64Image.substring(from, to).trim();
fBase64Image = fBase64Image.replace(oldExtension, newExtension);
}
fBase64Image = fBase64Image.replaceAll("/.*;", "/" + extension + ";"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
}

Note that a replace operation based on the extension is not correct, as the substring with the extension can also appear at other places within the image string.

@@ -41,13 +41,12 @@ public void testAssertWidth() {

@Test
public void testSetExtension() {
// assertTrue(getTipImage().getIMGAttributes(19, 10).contains("png"));
assertTrue(getTipImage().setExtension("png").getBase64Image().contains("png"));

Choose a reason for hiding this comment

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

Testing the default.

Suggested change
assertTrue(getTipImage().setExtension("png").getBase64Image().contains("png"));
assertTrue(getTipImage().getBase64Image().contains("png"));

Comment on lines 98 to 104
public void testSetExtension() throws IOException {
assertTrue(getTipImage().setExtension("png").getURL().toString().contains("png"));
}

@Test
public void testSetExtension2() {
// assertTrue(getTipImage().setExtension("bmp").getBase64Image().contains("bmp"));
// assertTrue(getTipImage().getIMGAttributes(19, 10).contains("png"));
public void testSetExtension2() throws IOException {
assertTrue(getTipImage().setExtension("bmp").getURL().toString().contains("bmp"));

Choose a reason for hiding this comment

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

I would not expect the URL to change but only the Base64 contents. So tests should be equal to the ones in TipImageBase64Test with the only difference that the image is initialized via URL rather than via Base64 string.

@Michael5601 Michael5601 force-pushed the TipImageTests branch 4 times, most recently from 991d97c to 40c2e9f Compare September 25, 2023 14:43
…-platform#525

In this commit the tests setExtension and setExtension2 are reactivated and changed. For this the method setExtension of TipImage.java is corrected to properly change the extension. Now if setExtension is called, not only the field fExtension is updated but also the field fBase64Image. Mind that for this change the field fBase64Image can't be final. Contributes to eclipse-platform#525.
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