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

Migrate CI to GitHub Actions #429

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Migrate CI to GitHub Actions #429

merged 4 commits into from
Jun 16, 2021

Conversation

gilest
Copy link
Collaborator

@gilest gilest commented Apr 15, 2021

Relatively similar to the Travis CI config.

Closes #465

Checklist:

  • Get tests passing
    • There's a test failure in the mirage-handler test that checks for errors emitted from uploading an invalid audio file. It's expected that an error from within a promise is elevated to ember's error handler – but this doesn't happen in some builds.
    • Doesn't occur in my local test runs. Can sort-of reproduce by adding a small timeout or a debugger
      here
    • We have decided to skip this test as we don't currently have the resources to trace the cause
  • Add deployment step
    • Setup workflow
    • Configure DEPLOY_KEY
    • Verify a test deploy

Deployed preview site here

It seems to be equivalent to the most recent master deploy.

@gilest gilest requested a review from jelhan April 15, 2021 03:59
@gilest gilest force-pushed the github-actions branch 5 times, most recently from d6f6bd4 to 16816e5 Compare April 15, 2021 07:20
@jelhan
Copy link
Collaborator

jelhan commented Apr 15, 2021

Thanks a lot for starting to work on this.

Haven't configured the Code Climate coverage stuff – do we want to port this?

If we decide to not port it, we could use create-github-actions-setup-for-ember-addon. Missing support for Code Climate and similar customization of CI pipeline is the only reason I haven't migrated this addon yet. Using the script to create (and update) GitHub Actions workflows is also recommended by program guidelines for adopted ember addons.

@gilest gilest force-pushed the github-actions branch 2 times, most recently from 326e5cb to c1ec916 Compare May 2, 2021 05:51
gilest added a commit that referenced this pull request May 2, 2021
As discussed in:

Add Github Actions workflow #429
Upgrade to Ember 3.26 #430
We're not sure Code Climate is up to date with the latest linting configuration.

It's also a soft-blocker for migrating the CI to GitHub Actions.
@gilest gilest force-pushed the github-actions branch 6 times, most recently from be0a2a6 to 14cbd15 Compare May 3, 2021 06:33
@gilest
Copy link
Collaborator Author

gilest commented May 3, 2021

@jelhan

I'm struggling to fix this one issue.

There's a test failure in the mirage-handler test that checks for errors emitted from uploading an invalid audio file. It's expected that an error from within a promise is elevated to ember's error handler – but this doesn't happen in some builds.

Doesn't occur in my local test runs. Can sort-of reproduce by adding a small timeout or a debugger here.

I've tried:

  • Also listening to RSVP.on('error')
  • Upgrading to ember-qunit 5
  • Updaing ember-cli-mirage and miragejs
  • Many, many dependency changes (see latest ember try builds). As far as I can tell test failure is not consistent with specific packages...
  • Regenerating yarn.lock

@jelhan
Copy link
Collaborator

jelhan commented May 3, 2021

I'm struggling to fix this one issue.

There's a test failure in the mirage-handler test that checks for errors emitted from uploading an invalid audio file. It's expected that an error from within a promise is elevated to ember's error handler – but this doesn't happen in some builds.

Doesn't occur in my local test runs.

Tests, which fail on CI but not locally, are the worst. So challenging to debug...

This is the test, which is failing:

const SCENARIOS = {
audio: new File(['invalid-data-for-a-video'], 'audio.mp3', { type: 'audio/mpeg' }),
image: new File(['invalid-data-for-an-image'], 'image.png', { type: 'image/png' }),
video: new File(['invalid-data-for-a-video'], 'video.webm', { type: 'video/webm' })
};
for (let [type, file] of Object.entries(SCENARIOS)) {
test(`upload handler throws if invalid ${type} is provided`, async function(assert) {
let errorCount = 0;
setupOnerror((error) => {
// expect two errors:
// 1. error thrown by our upload handler
// 2. error thrown by 500 response that includes the first error as body
if (errorCount === 0) {
// error thrown by our mirage handler
assert.step('error thrown');
assert.ok(error.message.includes(`invalid ${type}`));
} else {
// error from 500 response
assert.step('500 response');
assert.equal(error.status, 500);
assert.ok(error.body.error.includes(`invalid ${type}`));
}
errorCount++;
});
this.server.post('/image', uploadHandler(() => {}));
this.set('uploadImage', (file) => {
return file.upload('/image');
});
await render(hbs`
{{#file-upload
name="file"
onfileadd=uploadImage
}}
<a class="button">
Upload file
</a>
{{/file-upload}}
`);
await selectFiles('input', file);
assert.verifySteps(['error thrown', '500 response']);
});
}

The same test code is run for three scenarios: audio, image and video. Only audio is failing.

Audio and video are handled very similar in upload handler provided for mirage:

video(file, metadata) {
let video = document.createElement('video');
return new RSVP.Promise(function (resolve, reject) {
video.addEventListener('loadeddata', resolve);
video.onerror = () => {
reject(
new Error(
'You tried to upload an invalid video. The upload handler for mirage ' +
'shipped with ember-file-upload does not support invalid videos. ' +
'Please make sure that your video is valid and can be parsed by browsers.'
)
);
};
video.src = metadata.url;
document.body.appendChild(video);
video.load();
}).then(function () {
return {
duration: video.duration,
width: video.videoWidth,
height: video.videoHeight
};
}).finally(function () {
document.body.removeChild(video);
});
},
audio(file, metadata) {
let audio = document.createElement('audio');
return new RSVP.Promise(function (resolve, reject) {
audio.addEventListener('loadeddata', resolve);
audio.onerror = () => {
reject(
new Error(
'You tried to upload an invalid audio file. The upload handler for mirage ' +
'shipped with ember-file-upload does not support invalid audio files. ' +
'Please make sure that your audio is valid and can be parsed by browsers.'
)
);
};
audio.src = metadata.url;
document.body.appendChild(audio);
audio.load();
}).then(function () {
return {
duration: audio.duration
};
}).finally(function () {
document.body.removeChild(audio);
});
}
One creates a <audio> the other a <video> element. The video function reads videoWidth and videoHeight attributes additionally. I would be surprised if that small difference causes one to pass and the other to fail.

I was wondering if the CI is flickering or fails always for the same scenarios. I will restart several times and note which scenarios are passing and which are failing.

scenario first run second run third run fourth run
release ✔️
3.24.3 ✔️
3.20.6
3.16.10
3.12.4
3.8.3 ✔️ ✔️

It is not consistent. The same scenarios were passing in some runs but failing in other runs. This makes it even harder to debug. 😖

Can sort-of reproduce by adding a small timeout or a debugger here.

This sounds very much like a timing issue. I guess the timing between the different steps is not predictable. Will try to debug that next.

@gilest gilest force-pushed the github-actions branch from 14cbd15 to 7d5275b Compare May 4, 2021 07:52
@jelhan
Copy link
Collaborator

jelhan commented May 27, 2021

@gilest I'm wondering if #430 might fix the CI. At least TravisCI passed for that feature branch but is failing on master. Maybe we land #430 first and migrate to GitHub Actions afterwards?

@gilest
Copy link
Collaborator Author

gilest commented May 31, 2021

@gilest I'm wondering if #430 might fix the CI. At least TravisCI passed for that feature branch but is failing on master. Maybe we land #430 first and migrate to GitHub Actions afterwards?

Yes, let's try that. We need to keep moving forward anyway!

@jelhan
Copy link
Collaborator

jelhan commented May 31, 2021

@gilest I merged #430. Would be great if you could rebase / merge latest master in.

@gilest gilest force-pushed the github-actions branch 2 times, most recently from ebf5533 to f125b83 Compare June 1, 2021 22:30
@gilest
Copy link
Collaborator Author

gilest commented Jun 1, 2021

@jelhan rebased and re-ran create-github-actions-setup-for-ember-addon

Seems like that one spec is still unreliable under GitHub Actions

@jelhan
Copy link
Collaborator

jelhan commented Jun 14, 2021

Seems like that one spec is still unreliable under GitHub Actions

I would tend to skip that test for now. It only covers mirage test helpers. And this is eating up too much of our limited time...

@jelhan jelhan mentioned this pull request Jun 14, 2021
Don't run linters in testem

Drop .travis.yml.

Update lint and test commands.
@gilest gilest force-pushed the github-actions branch 3 times, most recently from 7f6d4e2 to cb38c6a Compare June 15, 2021 05:05
@gilest
Copy link
Collaborator Author

gilest commented Jun 15, 2021

@jelhan Ok I've skipped that test and we have fields of green now ✅

Only thing remaining is to migrate the docs deployment.

I've configured a basic workflow.

Just need a deployment key to be generated and added to GitHub secrets. I didn't know who has the permissions to do this so I've asked for help on Discord.

@gilest gilest force-pushed the github-actions branch 8 times, most recently from 522a0a2 to 273c4fb Compare June 16, 2021 02:36
@gilest
Copy link
Collaborator Author

gilest commented Jun 16, 2021

@jelhan I believe this is ready now 🙌

Have tested a docs deploy with the new configuration and it worked as expected.

Please review 👀

Copy link
Collaborator

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Looks good for me. Only a small question if we should maybe skip not all but only one of the tests for upload handler. But I may have missed something.

tests/integration/components/mirage-handler-test.js Outdated Show resolved Hide resolved
@gilest gilest changed the title Add Github Actions workflow Migrate CI to GitHub Actions Jun 16, 2021
@gilest gilest merged commit b5ab25f into master Jun 16, 2021
@gilest gilest deleted the github-actions branch June 16, 2021 12:29
@jelhan
Copy link
Collaborator

jelhan commented Jun 21, 2021

The deployment of docs failed when triggered by v4.0.0 release: https://github.com/adopted-ember-addons/ember-file-upload/runs/2873915337 @gilest Would be great if you could have a look what is going on.

@jelhan jelhan mentioned this pull request Jun 21, 2021
22 tasks
@gilest
Copy link
Collaborator Author

gilest commented Jun 21, 2021

@jelhan Ah yes – looks like the branch and tag deploys raced. Re-ran the workflow and it deployed fine.

Permanent fix in #481

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate CI
2 participants