Skip to content

compose: Skip failing test for windows. #1195

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

Conversation

apoorvapendse
Copy link
Contributor

@apoorvapendse apoorvapendse commented Dec 21, 2024

The attach from camera test fails on
Windows due to the the path separator
used in the split function of the XFile.name
getter in cross_file being \ instead of /.
This results in the getter returning the
entire path on Windows instead of the
last word present after the last '/' in the path.

Read this excellent explanation by
@chrisbobbe for better understanding:
https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/failing.20composebox.20test.20while.20running.20check.20script/near/2000301

@apoorvapendse apoorvapendse force-pushed the skip-failing-test-on-windows-due-to-path-separator branch 2 times, most recently from b1193df to 9048683 Compare December 23, 2024 07:36
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Dec 23, 2024
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Small comment below.

Also a commit message nit: let's make this the summary line:

compose_box [nfc]: On Windows, skip a test that fails

@apoorvapendse apoorvapendse force-pushed the skip-failing-test-on-windows-due-to-path-separator branch 3 times, most recently from c349d21 to 2494769 Compare December 24, 2024 02:23
@apoorvapendse
Copy link
Contributor Author

Thank you for the review @chrisbobbe .

Here is the change summary:

  1. Added a comment explaining the reason behind skipping this test on Windows as suggested here.
  2. Updated the commit message summary to the one recommended.
  3. Fixed the space after , here as suggested here.

@apoorvapendse apoorvapendse force-pushed the skip-failing-test-on-windows-due-to-path-separator branch from 2494769 to 576429c Compare December 24, 2024 02:25
@chrisbobbe
Copy link
Collaborator

Thanks! Marking for Greg's review; it's possible he'll want to tweak the explanation in the code comment, but the choice to skip this test on Windows seems fine to me.

@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 27, 2024
@chrisbobbe chrisbobbe requested review from gnprice and removed request for chrisbobbe December 27, 2024 19:40
@apoorvapendse
Copy link
Contributor Author

Thanks! Marking for Greg's review; it's possible he'll want to tweak the explanation in the code comment, but the choice to skip this test on Windows seems fine to me.

Thanks for the review @chrisbobbe as well as your simple explanation of why it fails on Windows.

Skips the test that fails because of the platform
specific path separator being '\' instead of '/
on Windows.

Read this excellent explanation by
@chrisbobbe for better understanding:
https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/failing.20composebox.20test.20while.20running.20check.20script/near/2000301

[greg: rewrote comment]
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks to you both!

it's possible he'll want to tweak the explanation in the code comment

Indeed. :-) In fact I also went and filed an upstream issue for the underlying bug in [XFile.name]:

Merging, after those adjustments:

       // TODO test what happens when capturing/uploading fails
-
-      // This test fails on Windows due to the Platform.pathSeparator.
-      // used in the split function of the XFile.name getter in cross_file being '\' instead of '/'.
-      // This results in the getter returning the entire path on Windows instead of the
-      // last word present after the last '/' in the path.
-    }, skip: Platform.isWindows);
+    },
+    // This test fails on Windows because [XFile.name] splits on
+    // [Platform.pathSeparator], corresponding to the actual host platform
+    // the test is running on, instead of the path separator for the
+    // target platform the test is simulating.
+    // TODO(upstream): unskip after fix to https://github.com/flutter/flutter/issues/161073
+    skip: Platform.isWindows);
   });

Comment on lines 589 to 590
// last word present after the last '/' in the path.
}, skip: Platform.isWindows);
Copy link
Member

Choose a reason for hiding this comment

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

nit: this puts the comment inside the test body function, which is potentially confusing about what the comment is referring to; instead, put it after the }, so it's clearly attached to the skip: argument

@gnprice gnprice force-pushed the skip-failing-test-on-windows-due-to-path-separator branch from 576429c to c02d947 Compare January 2, 2025 23:52
@gnprice gnprice merged commit c02d947 into zulip:main Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants