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/migrate wp desktop teamcity #47368

Merged
merged 14 commits into from
Nov 16, 2020
Merged

Add/migrate wp desktop teamcity #47368

merged 14 commits into from
Nov 16, 2020

Conversation

scinos
Copy link
Contributor

@scinos scinos commented Nov 12, 2020

Changes proposed in this Pull Request

  • Create TeamCity build for wp-desktop
  • Add junit reporter for mocha so tests can be reported in TeamCity
  • Add electron-*.log file to log Electron start up messages

Testing instructions

  • Go to TeamCity and verify there is a Desktop e2e tests build for this branch and it is passing.
  • Unfortunately not all features can be tested (eg: github notifications, tests reports) until this is merged (TeamCity doesn't like adding new features {} in a branch to an existing build).

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

desktop/e2e/run.js Outdated Show resolved Hide resolved
@scinos scinos force-pushed the add/migrate-wp-desktop-teamcity branch from b8cb03d to 8819395 Compare November 13, 2020 05:06
param("xmlReportParsing.reportType", "junit")
param("xmlReportParsing.reportDirs", "desktop/e2e/result.xml")
}
perfmon {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a tab with info about CPU/Mem used by the build

filterAuthorRole = PullRequests.GitHubRoleFilter.EVERYBODY
}
}
commitStatusPublisher {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a notification in GitHub PR with the status of this build

@@ -18,6 +18,8 @@ const APP_ARGS = [
'--disable-http-cache',
'--start-maximized',
'--remote-debugging-port=9222',
'--disable-setuid-sandbox',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this Chrome throws lots of problems. This was also done in CircleCI at the image config in https://github.com/CircleCI-Public/circleci-dockerfiles/blob/master/node/images/12.18.4-stretch/browsers/Dockerfile#L46

@scinos scinos requested a review from a team November 13, 2020 05:19
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 13, 2020
@scinos scinos self-assigned this Nov 13, 2020
@scinos scinos marked this pull request as ready for review November 13, 2020 05:19
Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-mac".

@scinos please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@wp-desktop wp-desktop dismissed their stale review November 13, 2020 06:08

wp-desktop ci passing, closing review

@@ -31,6 +31,10 @@ exports.createDir = function ( dir ) {

exports.isVideoEnabled = function () {
const video = process.env.CI;
// eslint-disable-next-line no-console
Copy link
Contributor

@nsakaimbo nsakaimbo Nov 13, 2020

Choose a reason for hiding this comment

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

Curious if including L34-L37 was intentional here? (I'm guessing this is useful to persist to the CI process logs for debugging?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!
Leftovers from when I was trying to debug video recorder, deleted.

@nsakaimbo
Copy link
Contributor

Looks really good! Couple of quick thoughts/question:

Is a "desktop" e2e CI badge supposed to be present on this PR? I only see the standard Calypso checks right now (I'm assuming this needs to get merged first?).

With your changes/additions to the linux container, I wonder if video capture for Linux is finally working? I attempted this a while ago - I got pretty far but ultimately failed (see here). Context: during my initial attempt, a file was saved but for some reason couldn't be viewed, so something was off somewhere.

Not a blocker for this PR at all but probably something I'll revisit at some point. The artifact is persisted to the desktop/e2e/screenshots directory - probably doesn't hurt to persist that location for inspection.

@nsakaimbo nsakaimbo self-requested a review November 13, 2020 22:52
Copy link
Contributor

@nsakaimbo nsakaimbo left a comment

Choose a reason for hiding this comment

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

Thanks for your amazing and hard work here, @scinos! ❤️

I know this was very much a non-trivial effort and there were a number of technical hurdles to overcome.

Looking forward to getting this integrated into wp-calypso and streamlining the build/CI process for everyone!

@scinos
Copy link
Contributor Author

scinos commented Nov 16, 2020

Is a "desktop" e2e CI badge supposed to be present on this PR? I only see the standard Calypso checks right now (I'm assuming this needs to get merged first?).

I got the message "Unsupported change of build features in the build configuration" in TeamCity, I assume those will start working once we merge it.

With your changes/additions to the linux container, I wonder if video capture for Linux is finally working

I tried :) When it tries to spawn ffmpeg it fails with Unknown input format: 'avfoundation'. Google tells me that's a MacOS thing and we may need to refactor how video-recorder works to account for the OS. I was planning to revisit it in a followup PR.

The artifact is persisted to the desktop/e2e/screenshots directory

It should be persisted as soon as we generate any content. These are the dirs we persist (from https://github.com/Automattic/wp-calypso/pull/47368/files#diff-660a0483d27a7df9ef700886a52d2714ac135f25af6889269798e06d37fff664R665-R669)

desktop/release
desktop/e2e/logs
desktop/e2e/screenshots

@scinos scinos merged commit 7ac5bd1 into master Nov 16, 2020
@scinos scinos deleted the add/migrate-wp-desktop-teamcity branch November 16, 2020 09:25
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 16, 2020
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.

4 participants