Skip to content

Add image block tests#1060

Merged
JavonDavis merged 18 commits intodevelopfrom
add/tests-image-block
Jul 9, 2019
Merged

Add image block tests#1060
JavonDavis merged 18 commits intodevelopfrom
add/tests-image-block

Conversation

@JavonDavis
Copy link
Copy Markdown
Contributor

@JavonDavis JavonDavis commented Jun 4, 2019

Part of #745

To test: New tests are in gutenberg-editor-image.test.js
If you'd like to run those alone locally a quick way to do that is change the describe block of the other test files to xdescribe

Alternatively run all the tests, they should all pass

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@JavonDavis JavonDavis added the Testing Anything related to automated tests label Jun 4, 2019
@JavonDavis JavonDavis self-assigned this Jun 5, 2019
@JavonDavis JavonDavis requested review from Tug and hypest June 7, 2019 19:36
@JavonDavis JavonDavis marked this pull request as ready for review June 7, 2019 19:36
@hypest hypest requested review from mchowning and removed request for Tug and hypest July 4, 2019 07:28
Copy link
Copy Markdown
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

Nice job @JavonDavis ! Everything works great and I think this is good to merge (once the conflict is resolved)! 🎉🚢

I added a couple of comments, but they're not issues with the code as much as just me trying to learn more about the codebase. 😀

await editorPage.addNewImageBlock();
let imageBlock = await editorPage.getImageBlockAtPosition( 1 );

if ( ! isAndroid() ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just asking for my own learning, why do our Android tests not need any of the code in these (rather large) blocks?

Copy link
Copy Markdown
Contributor Author

@JavonDavis JavonDavis Jul 9, 2019

Choose a reason for hiding this comment

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

On the emulator, clicking add image doesn't add an image to the image block, so that's why when running the test on iOS (Add Image > WordPress media library) it actually adds an image but on Android it doesn't. 🤔 I could probably move this code into a function to make it a little less obvious, WDYT?

Copy link
Copy Markdown
Contributor

@mchowning mchowning Jul 9, 2019

Choose a reason for hiding this comment

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

Thanks for explaining! That makes sense.

I do think it would be helpful to add a bit more context where we're doing these !isAndroid() checks. I don't have any strong preference between just adding a comment versus moving this code into a function, or even just creating a local "alias" along the lines of isCapableOfAddingImageToImageBlock() = () => ! isAndroid();, which would then be pretty descriptive if we used it in place of the !isAndroid() checks. Whatever you think would work best is fine with me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just opted for more context in the comments

//TODO: Improve the identifier for this element
return isAndroid() ?
await this.driver.elementByXPath( '//android.view.ViewGroup[@content-desc="Post title. Welcome to Gutenberg!"]/android.widget.EditText' ) :
await this.driver.elementByXPath( '//XCUIElementTypeOther[@name="Add title"]/XCUIElementTypeTextView' );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Certainly not something that needs to be changed, but FWIW in code like this I generally find it nice to separate out the things that vary (the path) from the things that don't (method call).

const path = isAndroid() ? 
        '//android.view.ViewGroup[@content-desc="Post title. Welcome to Gutenberg!"]/android.widget.EditText' ) : 
        '//XCUIElementTypeOther[@name="Add title"]/XCUIElementTypeTextView' );
return await this.driver.elementByXPath(path);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ahh you're right I I usually go with that approach, thanks I can change this

@JavonDavis JavonDavis merged commit 57b9613 into develop Jul 9, 2019
@SergioEstevao SergioEstevao deleted the add/tests-image-block branch January 14, 2020 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Anything related to automated tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants