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

Add Image Caption Styling #847

Conversation

mchowning
Copy link
Contributor

Updating the gutenberg submodule ref to include the changes for adding image caption styling. Description of the changes is in the associated gutenberg PR.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 9, 2019

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@etoledom etoledom self-requested a review April 9, 2019 17:50
@etoledom etoledom added the [Type] Enhancement Improves a current area of the editor label Apr 9, 2019
@etoledom
Copy link
Contributor

etoledom commented Apr 9, 2019

Hey @mchowning !

I tried to run the branch but I'm getting this error:

Screenshot_20190409-163007

If you don't see it locally, you can try yarn clean:install and yarn start:reset.
That should clean everything up. It also could be a local problem on my side, so if everything still works well for you, please let me know.

The error itself doesn't seem to be related to your changes. Keeping the branches updated from the base branches usually help in these cases 👍

Let me know!

@mchowning mchowning force-pushed the issue/image_caption_styling branch from efa01ef to bc76bc2 Compare April 10, 2019 01:52
@mchowning
Copy link
Contributor Author

Hey @etoledom !

Not sure how this ended up in that state. Anyway, should be good to go now. Sorry about that.

@etoledom
Copy link
Contributor

Thank you for the update @mchowning . It's working now! 🎉

@mchowning mchowning force-pushed the issue/image_caption_styling branch 3 times, most recently from 308eabe to f39d271 Compare April 16, 2019 12:30
@mchowning mchowning force-pushed the issue/image_caption_styling branch 2 times, most recently from c0e3a1c to cb7f9ad Compare April 26, 2019 02:22
@mchowning mchowning changed the base branch from develop to feature/caption April 27, 2019 00:04
@mchowning mchowning force-pushed the issue/image_caption_styling branch from cb7f9ad to 3b12ec3 Compare April 27, 2019 00:06
@mchowning
Copy link
Contributor Author

Switched this PR to target a feature/caption feature branch at @etoledom 's suggestion.

@etoledom
Copy link
Contributor

Thank you @mchowning !

Now that WordPress/gutenberg#14883 was merged into a local branch, updating the Gutenberg reference should make the CI run properly 🤞

@mchowning mchowning force-pushed the issue/image_caption_styling branch from 3b12ec3 to b895a2e Compare April 27, 2019 10:12
@mchowning
Copy link
Contributor Author

mchowning commented Apr 28, 2019

Looking into the test failures (ios, android), I'm seeing intermittent test failures locally on Android with the same "The environment you requested is not available" message that Circle CI reports. The error seems to often be device specific: (my physical Pixel 3 (Android 9.0) seems to always fails, but an emulated Nexus 5X (Android 9.0), and emulated Nexus S (Android 8.0) always pass. The only device that gives inconsistent results is my physical Pixel 1 device (Android 9.0), which passes approximately 90% of the time.

It does sound like these kinds of failures are a known issue based on this from the README's section on UI Tests:

Note, you might experience problems that seem to be related to the tests starting the Appium server, for example errors that say... The requested environment is not available. Sorry about that this is still a WIP, you can manually start the Appium server via appium desktop or the cli, then change the port number in the tests while optionally commenting out related code in the beforeAll and afterAll block.

This makes me wonder if these failures might be due to some flakiness in the tests. It seems unlikely that it would be flaky on both platforms at the same time, and I don't see any other builds that failed with this issue recently on Circle CI, so I'm far from convinced that this is just test flakiness. But, since I do get intermittent failures locally, I think it's worth retriggering the builds to see what a second run on Circle CI gets us.

What do you think @etoledom?

@etoledom
Copy link
Contributor

Thank you for the investigation @mchowning !
I'm retriggering the builds and cc @JavonDavis

Putting this up to observe CI e2e test behavior
@mchowning mchowning force-pushed the issue/image_caption_styling branch from b895a2e to 331c8be Compare April 29, 2019 14:42
@mchowning
Copy link
Contributor Author

mchowning commented Apr 29, 2019

Tests still failed on the rebuild (ios, android).

As a sanity check I force pushed to the source branch for this PR (mchowning:issue/image_caption_styling), reverting my update to the gutenberg ref (so the branch would be unchanged), and adding a commit that does nothing but add an empty text file, and the UI tests still fail even with only that non-code change (ios, android). So Circle CI just doesn't like me!

Sanity check:

  • The target branch (feature/caption) and develop are both on commit 42d8443 at this time.
  • The source branch is identical to that except for the b895a2e commit I just added, which only adds a small text file to the root of the project (just wanted something in there so there would be a diff between the branches).

I want to say that the problem is on develop, but I don't see any other builds having this issue. (recent change to develop maybe?)

🚨 This PR should NOT be merged until I revert the change I made to this PR so that it only adds an empty text file.

@mchowning
Copy link
Contributor Author

mchowning commented Apr 29, 2019

@etoledom helped confirm that the UI tests were failing because this PR's source branch was coming from a fork. @etoledom has taken care of merging the code from b895a2e (the commit before I force pushed a test commit to this PR) into the feature/caption branch in #940, so closing this PR.

It appears that the UI tests were failing on this PR because the source branch for the PR was from a fork. @eduardo's PR was based on a branch within gutenberg-mobile that he created, and the tests passed.

@mchowning mchowning closed this Apr 29, 2019
@mchowning mchowning deleted the issue/image_caption_styling branch April 29, 2019 19:05
@JavonDavis
Copy link
Contributor

Thanks for helping out on this @etoledom!

@JavonDavis
Copy link
Contributor

Looking into the test failures (ios, android), I'm seeing intermittent test failures locally on Android with the same "The environment you requested is not available" message that Circle CI reports. The error seems to often be device specific: (my physical Pixel 3 (Android 9.0) seems to always fails, but an emulated Nexus 5X (Android 9.0), and emulated Nexus S (Android 8.0) always pass. The only device that gives inconsistent results is my physical Pixel 1 device (Android 9.0), which passes approximately 90% of the time.

@mchowning thanks a lot for highlighting this, I've made a note of it... There are still some things to work out with the environment startup and this is definitely something we're planning to address. Could you confirm that for the failures the only error you saw was The environment you requested is not available or something that seemed related to that? Just wanted to make sure that there weren't any failures with the tests themselves on the different devices for you, thanks 🙂

@mchowning
Copy link
Contributor Author

Could you confirm that for the failures the only error you saw was The environment you requested is not available or something that seemed related to that?

👋 @JavonDavis ! Yes, the only failure I observed was the "The environment you requested is not available" failure.

It did seem like my Pixel 1 device was more likely to fail when I ran the tests immediately after restarting my computer. I tried to confirm this, and it definitely did not always fail after restarting my computer though, so it is entirely possible that I just imagined that correlation.

Let me know if there's any testing I can do to help you out tracking this down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants