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

Warn on RunTask failures #93

Merged
merged 3 commits into from
Jun 16, 2024
Merged

Conversation

liath
Copy link
Contributor

@liath liath commented Jun 12, 2024

Might want to also log, I'll have to devise a way to cause the API to fail to test this

@liath liath requested a review from a team June 12, 2024 20:50
@liath
Copy link
Contributor Author

liath commented Jun 13, 2024

Added unittests across index, not terribly proud of them though

index.js Outdated
@@ -74,6 +74,12 @@ module.exports = function (options, cb) {
return taskRunner.runPromisified(params);
})
.then((taskDefinition) => {
if (taskDefinition.failures) {
console.error("Task failed to launch:")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to just throw the error - anything interested should catch it and log as appropriate. This is intended to be used as a library after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

package.json Outdated
@@ -10,7 +10,7 @@
"ecs-task-runner": "./bin/ecs-task-runner"
},
"scripts": {
"test": "NODE_ENV=test ./node_modules/.bin/mocha --recursive"
"test": "NODE_ENV=test ./node_modules/.bin/mocha --parallel --recursive"
Copy link
Contributor

Choose a reason for hiding this comment

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

would avoid parallel usage, I remember some problems with one of these codebases where it was not really concurrency safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this is one of them apparently. I removed parallel and now tests won't pass lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m-radzikowski/aws-sdk-client-mock#64
Apparently mocha has weird behavior with aws-sdk-client-mock, shuffling up the mockClient invocations resolves it.

});

try {
await index(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not something like expect(index(options).to.throw ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't work ¯_(ツ)_/¯
expect.js hasn't been updated in a decade, maybe they don't support promises?

Copy link
Contributor Author

@liath liath Jun 13, 2024

Choose a reason for hiding this comment

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

I think mocha and expect.js might be kind dead projects. Something that annoyed my the whole time in working on this is that errors were always "test timed out" and apparently that's just mocha being terrible with promises.
mochajs/mocha#2640

ecsMock.on(RunTaskCommand).callsFake(async params => {
expect(params.taskDefinition).to.eql(options.taskDefinitionArn)

eofSet(params.overrides.containerOverrides[0].command[2].split(' ')[9])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really get what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment, but yeah this is ugliest bit of this. This is the only place we can grab the random value the log stuff uses to detect the end of the task run.

@liath liath merged commit 9417a7b into master Jun 16, 2024
1 check passed
@liath liath deleted the infreq-643-warn-on-RunTask-failures branch June 16, 2024 18:27
@liath liath changed the title [INFREQ-643] Warn on RunTask failures Warn on RunTask failures Jun 19, 2024
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.

3 participants