Conversation
killian-mathias
force-pushed
the
fix/tests
branch
from
8aa0c30 to
e94b078
Compare
PR Reviewer Guide 🔍(Review updated until commit 2707353)Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| photospickerApp/*@START_MENU_TOKEN@*/ | ||
| .images["Photo, 30 mars 2018, 21:14"]/*[[".otherElements.images[\"Photo, 30 mars 2018, 21:14\"]",".images[\"Photo, 30 mars 2018, 21:14\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/ | ||
| .coordinate(withNormalizedOffset: CGVector( | ||
| dx: 0, | ||
| dy: 0 | ||
| )).tap() |
There was a problem hiding this comment.
Suggestion: The hardcoded photo date "30 mars 2018, 21:14" assumes a specific photo exists in the library, which will cause test failures on different simulators or devices. Use photospickerApp.images.firstMatch or select by index to choose any available photo. [general, importance: 7]
| photospickerApp/*@START_MENU_TOKEN@*/ | |
| .images["Photo, 30 mars 2018, 21:14"]/*[[".otherElements.images[\"Photo, 30 mars 2018, 21:14\"]",".images[\"Photo, 30 mars 2018, 21:14\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/ | |
| .coordinate(withNormalizedOffset: CGVector( | |
| dx: 0, | |
| dy: 0 | |
| )).tap() | |
| photospickerApp.images.firstMatch | |
| .coordinate(withNormalizedOffset: CGVector( | |
| dx: 0.5, | |
| dy: 0.5 | |
| )).tap() |
| // Leave office edition page | ||
| let officeBackButton = app.webViews.staticTexts["chevron_left_ios"] | ||
| XCTAssertTrue(officeBackButton.waitForExistence(timeout: 4), "back button should be displayed") | ||
| let officeBackButton = app.images.element(boundBy: 5) |
There was a problem hiding this comment.
Suggestion: Accessing UI elements by index is fragile and will break if the UI layout changes even slightly. Replace with a specific accessibility identifier or label to locate the back button reliably. [general, importance: 6]
| let officeBackButton = app.images.element(boundBy: 5) | |
| let officeBackButton = app.images["officeBackButton"].firstMatch |
There was a problem hiding this comment.
If we can manage to get the element by ID and not rely on hardcoded indexes it is indeed better 👍
|
Persistent review updated to latest commit dfdc0a7 |
| let documentFileURLs = try FileManager.default.contentsOfDirectory( | ||
| at: FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first!, | ||
| includingPropertiesForKeys: nil, | ||
| options: .skipsHiddenFiles | ||
| ) |
There was a problem hiding this comment.
Suggestion: Replace the force unwrap with safe optional binding to prevent a crash if the document directory URL cannot be retrieved. Use guard let to safely unwrap the optional array result. [possible issue, importance: 7]
| let documentFileURLs = try FileManager.default.contentsOfDirectory( | |
| at: FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first!, | |
| includingPropertiesForKeys: nil, | |
| options: .skipsHiddenFiles | |
| ) | |
| guard let documentDirectory = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first else { | |
| return | |
| } | |
| let documentFileURLs = try FileManager.default.contentsOfDirectory( | |
| at: documentDirectory, | |
| includingPropertiesForKeys: nil, | |
| options: .skipsHiddenFiles | |
| ) |
There was a problem hiding this comment.
You can do it as well to explicit the issue in console
| let photospickerApp = XCUIApplication(bundleIdentifier: "com.apple.mobileslideshow.photospicker") | ||
| photospickerApp/*@START_MENU_TOKEN@*/ | ||
| .images["Photo, 30 mars 2018, 21:14"]/*[[".otherElements.images[\"Photo, 30 mars 2018, 21:14\"]",".images[\"Photo, 30 mars 2018, 21:14\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/ | ||
| .coordinate(withNormalizedOffset: CGVector()).tap() |
There was a problem hiding this comment.
Suggestion: Specify the center coordinates using CGVector(dx: 0.5, dy: 0.5) instead of the default zero vector. The default CGVector() creates a zero offset (top-left corner), which may miss the interactive area of the photo element. [possible issue, importance: 6]
| .coordinate(withNormalizedOffset: CGVector()).tap() | |
| .coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
|
I prepared my review, we'll discuss it tomorrow 🙌 |
adrien-coye
left a comment
adrien-coye
left a comment
There was a problem hiding this comment.
We did review this PR face to face.
It looks good, but I'd like to go further with this PR :
- CI step
- Switch to EN-UK
- A couple of small remarks
| @@ -301,30 +372,33 @@ class AppUITest: XCTestCase { | |||
| } | |||
|
|
|||
| func testCreateSharedDirectory() { | |||
There was a problem hiding this comment.
For every "create" task, I would like to see a cleanup function to remove said file.
It could be a direct call to the API to make sure the test account stays relatively clean.
| let documentFileURLs = try FileManager.default.contentsOfDirectory( | ||
| at: FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first!, | ||
| includingPropertiesForKeys: nil, | ||
| options: .skipsHiddenFiles | ||
| ) |
There was a problem hiding this comment.
You can do it as well to explicit the issue in console
|
Persistent review updated to latest commit eccd2ad |
|
Persistent review updated to latest commit 7a5e315 |
| // MARK: - Tests setup | ||
|
|
||
| func testLogin() { | ||
| setUp() |
There was a problem hiding this comment.
Suggestion: Do not manually invoke setUp() inside test methods. XCTest automatically calls this method before each test. Manual invocation causes double initialization of the test environment and XCUIApplication instance. Rename the custom configuration method (e.g., to configureTest()) and invoke that instead. [possible issue, importance: 8]
| setUp() | |
| func testLogin() { | |
| configureTest() | |
| launchAppFromScratch() | |
| login() | |
| } |
| let closeButton = app.buttons["Fermer"] | ||
| XCTAssertTrue(closeButton.waitForExistence(timeout: 3), "Close button should be visible") | ||
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | ||
| closeButton.tap() |
There was a problem hiding this comment.
Suggestion: Restore the existence check before tapping the button. The previous code verified the button existed with waitForExistence(timeout: 3), but this safety check was removed when localizing the string. Without this check, the test crashes if the button isn't immediately available. [possible issue, importance: 7]
| closeButton.tap() | |
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | |
| XCTAssertTrue(closeButton.waitForExistence(timeout: 3), "Close button should be visible") | |
| closeButton.tap() |
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | ||
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
There was a problem hiding this comment.
Suggestion: Fix the backwards assertion logic. The code asserts that the photo library is empty, but then immediately attempts to tap the second image element (index 1), which causes an out-of-bounds crash when the collection is empty. Change to XCTAssertFalse to ensure photos exist before tapping. [possible issue, importance: 9]
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() | |
| XCTAssertFalse(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
|
Persistent review updated to latest commit 9e408c6 |
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | ||
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
There was a problem hiding this comment.
Suggestion: The assertion logic is inverted - it fails when photos exist (the expected state) and would pass when empty, causing the subsequent element(boundBy: 1) access to crash with an out-of-bounds error. Replace with a count check to ensure at least one photo exists before tapping. [possible issue, importance: 9]
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() | |
| XCTAssertGreaterThan(photospickerApp.images.count, 0, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | ||
| closeButton.tap() |
There was a problem hiding this comment.
Suggestion: The removal of the waitForExistence check before the first closeButton.tap() removes a safety guard that ensures the UI element is ready. This may cause the test to tap a non-existent button if the UI hasn't finished transitioning, leading to intermittent failures. [possible issue, importance: 6]
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | |
| closeButton.tap() | |
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | |
| XCTAssertTrue(closeButton.waitForExistence(timeout: 3), "Close button should be visible") | |
| closeButton.tap() |
| let officeBackButton = app.images.element(boundBy: 5) | ||
| XCTAssertTrue(officeBackButton.waitForExistence(timeout: 10), "back button should be displayed") |
There was a problem hiding this comment.
Suggestion: Accessing UI elements by hardcoded index boundBy: 5 is fragile and will break if the UI layout changes or additional images are added. Replace with an accessibility identifier query to reliably locate the back button regardless of layout changes. [general, importance: 5]
| let officeBackButton = app.images.element(boundBy: 5) | |
| XCTAssertTrue(officeBackButton.waitForExistence(timeout: 10), "back button should be displayed") | |
| let officeBackButton = app.images["OfficeBackButton"] | |
| XCTAssertTrue(officeBackButton.waitForExistence(timeout: 10), "back button should be displayed") |
|
Persistent review updated to latest commit d376b5e |
| let photospickerApp = XCUIApplication(bundleIdentifier: "com.apple.mobileslideshow.photospicker") | ||
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") |
There was a problem hiding this comment.
Suggestion: The assertion verifies that no photos exist (isEmpty == true), but the subsequent line attempts to tap the second image (index 1), which will crash or fail when the library is empty. Change the assertion to verify images exist (isEmpty == false) and use index 0 to select the first available image. [possible issue, importance: 9]
| let photospickerApp = XCUIApplication(bundleIdentifier: "com.apple.mobileslideshow.photospicker") | |
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| XCTAssertFalse(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 0).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
| setUp() | ||
| launchAppFromScratch(resetData: false) | ||
|
|
||
| let laterButton = app.buttons[KDriveResourcesStrings.Localizable.buttonLater] | ||
| if laterButton.exists { |
There was a problem hiding this comment.
Suggestion: Remove the manual setUp() invocation inside test methods. setUp() is an XCTest lifecycle method automatically called before each test; invoking it manually reinitializes the app property and test state, leading to unpredictable behavior or crashes. [possible issue, importance: 7]
| setUp() | |
| launchAppFromScratch(resetData: false) | |
| let laterButton = app.buttons[KDriveResourcesStrings.Localizable.buttonLater] | |
| if laterButton.exists { | |
| func testRenameFile() { | |
| let testName = "UITest - Rename file-\(Date())" | |
| let newTestName = "\(testName)_update" | |
| launchAppFromScratch(resetData: false) |
| closeButton.tap() | ||
|
|
There was a problem hiding this comment.
Suggestion: Restore the waitForExistence timeout assertion before tapping the button. Without this wait, the test attempts to tap immediately, causing race condition failures if the button hasn't appeared in the view hierarchy yet. [possible issue, importance: 7]
| closeButton.tap() | |
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | |
| XCTAssertTrue(closeButton.waitForExistence(timeout: 3), "Close button should be visible") | |
| closeButton.tap() |
killian-mathias
force-pushed
the
fix/tests
branch
from
d376b5e to
65713e0
Compare
|
Persistent review updated to latest commit 65713e0 |
| UITEST_ACCOUNT_PASSWORD: ${{ secrets.UITEST_ACCOUNT_PASSWORD }} | ||
| run: | | ||
| touch $ENV_PATH | ||
| echo -e "enum Env {\n static let testAccountEmail = \"$UITEST_ACCOUNT_EMAIL\"\n static let testAccountPassword = \"$UITEST_ACCOUNT_PASSWORD\"\n}" > $ENV_PATH \ -testLanguage en-UK \ -testRegion UK |
There was a problem hiding this comment.
Suggestion: Remove the erroneously appended test arguments (-testLanguage and -testRegion) from the echo command. These appear to be copy-pasted from the subsequent test command and will cause invalid Swift syntax in the generated Env.swift file. [possible issue, importance: 9]
| echo -e "enum Env {\n static let testAccountEmail = \"$UITEST_ACCOUNT_EMAIL\"\n static let testAccountPassword = \"$UITEST_ACCOUNT_PASSWORD\"\n}" > $ENV_PATH \ -testLanguage en-UK \ -testRegion UK | |
| echo -e "enum Env {\n static let testAccountEmail = \"$UITEST_ACCOUNT_EMAIL\"\n static let testAccountPassword = \"$UITEST_ACCOUNT_PASSWORD\"\n}" > $ENV_PATH |
| func testLogin() { | ||
| setUp() | ||
| launchAppFromScratch() |
There was a problem hiding this comment.
Suggestion: Remove the explicit setUp() call from test methods. XCTest automatically invokes setUp() before each test method, so calling it manually causes double initialization which may reset app state unexpectedly. [general, importance: 7]
| func testLogin() { | |
| setUp() | |
| launchAppFromScratch() | |
| func testLogin() { | |
| launchAppFromScratch() |
There was a problem hiding this comment.
This comment is correct, some calls to setUp() are still present. This is called by the XCTest framework on its own.
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | ||
| closeButton.tap() |
There was a problem hiding this comment.
Suggestion: Restore the waitForExistence assertion before tapping the close button. Removing this check introduces a race condition where the tap may occur before the button is fully rendered and interactable. [general, importance: 6]
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | |
| closeButton.tap() | |
| let closeButton = app.buttons[KDriveResourcesStrings.Localizable.buttonClose] | |
| XCTAssertTrue(closeButton.waitForExistence(timeout: 3), "Close button should be visible") | |
| closeButton.tap() |
killian-mathias
force-pushed
the
fix/tests
branch
from
65713e0 to
6d81aba
Compare
|
Persistent review updated to latest commit 6d81aba |
| UITEST_ACCOUNT_PASSWORD: ${{ secrets.UITEST_ACCOUNT_PASSWORD }} | ||
| run: | | ||
| touch $ENV_PATH | ||
| echo -e "enum Env {\n static let testAccountEmail = \"$UITEST_ACCOUNT_EMAIL\"\n static let testAccountPassword = \"$UITEST_ACCOUNT_PASSWORD\"\n}" > $ENV_PATH \ |
There was a problem hiding this comment.
Suggestion: Remove the trailing backslash to prevent the shell command from escaping the newline and attempting to execute the next YAML line as part of the shell command, which would cause a workflow syntax error. [possible issue, importance: 9]
| echo -e "enum Env {\n static let testAccountEmail = \"$UITEST_ACCOUNT_EMAIL\"\n static let testAccountPassword = \"$UITEST_ACCOUNT_PASSWORD\"\n}" > $ENV_PATH \ | |
| echo -e "enum Env {\n static let testAccountEmail = \"$UITEST_ACCOUNT_EMAIL\"\n static let testAccountPassword = \"$UITEST_ACCOUNT_PASSWORD\"\n}" > $ENV_PATH |
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | ||
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
There was a problem hiding this comment.
Suggestion: The assertion verifies that the photo library is empty, but the subsequent line attempts to access the second image (index 1). This contradiction will cause the test to crash when accessing an empty collection. Remove the assertion or change it to verify that images exist before tapping. [possible issue, importance: 9]
| XCTAssertTrue(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() | |
| XCTAssertFalse(photospickerApp.images.debugDescription.isEmpty, "Photo library should not be empty") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
| UserDefaults.standard.removePersistentDomain(forName: Bundle.main.bundleIdentifier!) | ||
| UserDefaults.shared.removePersistentDomain(forName: Bundle.main.bundleIdentifier!) |
There was a problem hiding this comment.
Suggestion: Force unwrapping Bundle.main.bundleIdentifier could cause a crash if the identifier is nil (e.g., in certain test environments). Use optional binding to safely unwrap the value before passing it to the removal method. [possible issue, importance: 7]
| UserDefaults.standard.removePersistentDomain(forName: Bundle.main.bundleIdentifier!) | |
| UserDefaults.shared.removePersistentDomain(forName: Bundle.main.bundleIdentifier!) | |
| guard let bundleIdentifier = Bundle.main.bundleIdentifier else { return } | |
| UserDefaults.standard.removePersistentDomain(forName: bundleIdentifier) | |
| UserDefaults.shared.removePersistentDomain(forName: bundleIdentifier) |
killian-mathias
force-pushed
the
fix/tests
branch
from
6d81aba to
7fedeef
Compare
|
Persistent review updated to latest commit 7fedeef |
| - name: Setup | ||
| run: tuist install && tuist generate --no-open | ||
| - name: UITest | ||
| env: TEST_RUNNER_LAUNCH_ARGS: "-AppleLanguages (en-GB) -AppleLocale en_GB" |
There was a problem hiding this comment.
Suggestion: Fix the malformed YAML syntax for the env key. The current inline format may be parsed as a string rather than a mapping, causing the environment variable to not be set correctly. Use proper YAML mapping syntax with the variable name on a new indented line. [possible issue, importance: 9]
| env: TEST_RUNNER_LAUNCH_ARGS: "-AppleLanguages (en-GB) -AppleLocale en_GB" | |
| env: | |
| TEST_RUNNER_LAUNCH_ARGS: "-AppleLanguages (en-GB) -AppleLocale en_GB" |
| let shareAndRights = collectionViewsQuery.cells.staticTexts["Partage et droits"] | ||
| let shareAndRights = collectionViewsQuery.cells.staticTexts[KDriveResourcesStrings.Localizable.buttonFileRights] | ||
| shareAndRights.tap() | ||
| let directoryShareAndRights = app.navigationBars["Partage et droits du dossier \(root)"] |
There was a problem hiding this comment.
Suggestion: Replace the hardcoded French navigation bar title with the English equivalent or a localized string to match the en-GB locale configuration used in the tests. The app runs in English (en-GB) per the launch arguments, so checking for a French title will cause the test to fail. [possible issue, importance: 9]
| let directoryShareAndRights = app.navigationBars["Partage et droits du dossier \(root)"] | |
| let directoryShareAndRights = app.navigationBars["Share and rights of folder \(root)"] |
| .buttons["Add"]/*[[".navigationBars",".buttons[\"Terminé\"]",".buttons[\"Add\"]"],[[[-1,2],[-1,1],[-1,0,1]],[[-1,2],[-1,1]]],[0]]@END_MENU_TOKEN@*/ | ||
| .firstMatch.tap() | ||
| let buttonSave = app.buttons[KDriveResourcesStrings.Localizable.buttonSave] |
There was a problem hiding this comment.
Suggestion: Remove the Xcode test recorder placeholder comments (/*@START_MENU_TOKEN@*/ and associated metadata) from the UI test code. These artifacts indicate auto-generated code that should be cleaned up for better readability and maintainability. [general, importance: 4]
| .buttons["Add"]/*[[".navigationBars",".buttons[\"Terminé\"]",".buttons[\"Add\"]"],[[[-1,2],[-1,1],[-1,0,1]],[[-1,2],[-1,1]]],[0]]@END_MENU_TOKEN@*/ | |
| .firstMatch.tap() | |
| let buttonSave = app.buttons[KDriveResourcesStrings.Localizable.buttonSave] | |
| photospickerApp.buttons["Add"].firstMatch.tap() |
killian-mathias
force-pushed
the
fix/tests
branch
from
f8f7372 to
8cd0d0b
Compare
|
Persistent review updated to latest commit 8cd0d0b |
| app/*@START_MENU_TOKEN@*/ | ||
| .buttons[KDriveResourcesStrings.Localizable | ||
| .buttonBack]/*[[".navigationBars",".buttons[\"Filtres\"]",".buttons[\"BackButton\"]",".buttons"],[[[-1,2],[-1,1],[-1,3],[-1,0,1]],[[-1,2],[-1,1]]],[0]]@END_MENU_TOKEN@*/ | ||
| .firstMatch.tap() |
There was a problem hiding this comment.
Suggestion: Remove Xcode UI test recording artifacts (START_MENU_TOKEN comments). These are automatically generated by Xcode during test recording and should be cleaned up to maintain readable, maintainable code. Use a clean multi-line statement instead. [general, importance: 6]
| app/*@START_MENU_TOKEN@*/ | |
| .buttons[KDriveResourcesStrings.Localizable | |
| .buttonBack]/*[[".navigationBars",".buttons[\"Filtres\"]",".buttons[\"BackButton\"]",".buttons"],[[[-1,2],[-1,1],[-1,3],[-1,0,1]],[[-1,2],[-1,1]]],[0]]@END_MENU_TOKEN@*/ | |
| .firstMatch.tap() | |
| app.buttons[KDriveResourcesStrings.Localizable.buttonBack] | |
| .firstMatch.tap() |
| XCTAssertFalse(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | ||
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
There was a problem hiding this comment.
Suggestion: Replace the fragile debugDescription.isEmpty check with a proper count validation. The current code assumes at least 2 images exist (accessing index 1) but only checks if the description string is non-empty. Verify images.count > 1 before accessing a specific index to prevent crashes when the photo library has insufficient images. [possible issue, importance: 8]
| XCTAssertFalse(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() | |
| XCTAssertGreaterThan(photospickerApp.images.count, 1, "Need at least 2 photos in photo library") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
| app.staticTexts[testName].firstMatch.tap() | ||
| XCTAssertTrue( | ||
| app.staticTexts["20180330_211419_3650.jpeg"].waitForExistence(timeout: 3), | ||
| "Photo should be back in directory" |
There was a problem hiding this comment.
Suggestion: Avoid hardcoding the expected filename "20180330_211419_3650.jpeg" as it assumes a specific photo exists in the simulator's photo library. Store the actual filename returned from the photo picker during upload and use that variable for assertions, or verify the file exists by checking the cell count or a dynamic identifier. [possible issue, importance: 6]
| app.staticTexts[testName].firstMatch.tap() | |
| XCTAssertTrue( | |
| app.staticTexts["20180330_211419_3650.jpeg"].waitForExistence(timeout: 3), | |
| "Photo should be back in directory" | |
| // Store the filename from the upload flow and use it here | |
| XCTAssertTrue( | |
| app.staticTexts[uploadedPhotoName].waitForExistence(timeout: 3), | |
| "Photo should be back in directory" | |
| ) |
killian-mathias
force-pushed
the
fix/tests
branch
from
2258f23 to
b815d17
Compare
|
Persistent review updated to latest commit 2258f23 |
|
Persistent review updated to latest commit b815d17 |
killian-mathias
force-pushed
the
fix/tests
branch
from
b815d17 to
0876d74
Compare
killian-mathias
force-pushed
the
fix/tests
branch
from
0876d74 to
2707353
Compare
|
Persistent review updated to latest commit 2707353 |
| } | ||
| let photospickerApp = XCUIApplication(bundleIdentifier: "com.apple.mobileslideshow.photospicker") | ||
| XCTAssertFalse(photospickerApp.images.debugDescription.isEmpty, "No photos in photo library") | ||
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
There was a problem hiding this comment.
Suggestion: Add a bounds check before accessing element(boundBy: 1) to prevent crashes when the photo library contains fewer than 2 images. Accessing an index beyond the available elements will cause the UI test to crash. [possible issue, importance: 6]
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() | |
| XCTAssertGreaterThan(photospickerApp.images.count, 1, "Photo library needs at least 2 images") | |
| photospickerApp.images.element(boundBy: 1).coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).tap() |
| var deleteString = String() | ||
| for _ in newTestName { | ||
| deleteString += XCUIKeyboardKey.delete.rawValue | ||
| } | ||
| fileNameTextField.typeText(deleteString) |
There was a problem hiding this comment.
Suggestion: Calculate the delete string length based on the current text field content (root) rather than newTestName. The text field contains the existing folder name (root), so deleting newTestName.count characters may delete too many or too few characters, causing test failures or unexpected behavior. [possible issue, importance: 8]
| var deleteString = String() | |
| for _ in newTestName { | |
| deleteString += XCUIKeyboardKey.delete.rawValue | |
| } | |
| fileNameTextField.typeText(deleteString) | |
| var deleteString = String() | |
| for _ in root { | |
| deleteString += XCUIKeyboardKey.delete.rawValue | |
| } | |
| fileNameTextField.typeText(deleteString) |
adrien-coye
left a comment
adrien-coye
left a comment
There was a problem hiding this comment.
I did a new review of this PR.
We discussed how to work with the CI.
Beyond using a defer to make sure the cleanup is applied all the time, there is some extraneous setUp() calls that are not needed.
| passwordField.typeText(Env.testAccountPassword) | ||
| passwordField.typeText("\n") | ||
|
|
||
| // wait(delay: 5) |
There was a problem hiding this comment.
If we do not need it we should just remove it before we merge this PR.
| func testLogin() { | ||
| setUp() | ||
| launchAppFromScratch() |
There was a problem hiding this comment.
This comment is correct, some calls to setUp() are still present. This is called by the XCTest framework on its own.
|
Failed to generate code suggestions for PR |
killian-mathias
force-pushed
the
fix/tests
branch
from
42099e9 to
7983b46
Compare
killian-mathias
force-pushed
the
fix/tests
branch
from
7983b46 to
8658bc9
Compare
|
Failed to generate code suggestions for PR |
|
|
Failed to generate code suggestions for PR |
3 participants
Refonte des tests en intégrant les traductions, en créant des fonctions pour les tâches répétitives.