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 multiple attends to run package test suite #17169

Closed
Mati365 opened this issue Sep 27, 2024 · 2 comments · Fixed by #17219
Closed

Add multiple attends to run package test suite #17169

Mati365 opened this issue Sep 27, 2024 · 2 comments · Fixed by #17219
Assignees
Labels
intro Good first ticket. squad:platform Issue to be handled by the Platform team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Mati365
Copy link
Member

Mati365 commented Sep 27, 2024

📝 Provide a description of the improvement

Our test suite tends to randomly crash on certain packages without any reason. Most of these crashes are random disconnections from Karma server which result in Disconnected reconnect failed before timeout of 2000ms (ping timeout) error.

It's crashing, mostly in our commercial repo. There might be few reasons why it's crashing:

  1. Maybe due to async nature of the commercial tests. They are using async stuff and maybe some of the socket connections are not properly destroyed, causing tests to timeout. It'd be explanation why merging this branch: https://github.com/cksource/ckeditor5-commercial/commit/29d1c94cf5afd3d40cd9676d0f7bf853aacde953 caused more instability than it was before. Debugging it may be time-consuming.
  2. Maybe some breaking changes were introduced in latest Chrome 129 upgrade that does not play well with Karma. There was at least one issue with our testing suite after upgrade: cf5533f so some changes in protocol were made.

Proposal workaround

Let's add multiple attends to our scripts/ci/check-unit-tests-for-package.js calls in testing suite. In other words, let's rewrite these CI bash scripts:

#!/bin/bash -eo pipefail

node scripts/ci/check-unit-tests-for-package.js --package-name ckeditor5-ai --check-coverage

into something like that (I did not test it):

#!/bin/bash -eo pipefail

COMMAND="node scripts/ci/check-unit-tests-for-package.js --package-name ckeditor5-ai --check-coverage"

for attempt in {1..3}; do
    echo "Attempt $attempt..."
    $COMMAND && exit 0
done

echo "Command failed after 3 attempts."
exit 1

It's not a great solution, but good enough at this moment.


❓ Maybe CircleCI supports it out of the box and might be done at job level?

@Mati365 Mati365 added the type:improvement This issue reports a possible enhancement of an existing feature. label Sep 27, 2024
@pomek
Copy link
Member

pomek commented Sep 27, 2024

❓ Maybe CircleCI supports it out of the box and might be done at job level?

Note: CircleCI does not support automatic retries of failed steps, jobs & workflows. If you are looking for that feature, please vote for it here.

https://support.circleci.com/hc/en-us/articles/360043188514-How-to-Retry-a-Failed-Step-with-when-Attribute

@pomek pomek added squad:platform Issue to be handled by the Platform team. intro Good first ticket. labels Sep 27, 2024
@pomek
Copy link
Member

pomek commented Sep 27, 2024

A function to update:

/**
* @param {Object} options
* @param {String} options.packageName
* @param {Boolean} options.checkCoverage
*/
function runTests( { packageName, checkCoverage } ) {
const shortName = packageName.replace( /^ckeditor5?-/, '' );
const testCommand = [
'yarn',
'test',
'--reporter=dots',
'--production',
`-f ${ shortName }`,
checkCoverage ? '--coverage' : null
].filter( Boolean );
execSync( testCommand.join( ' ' ), {
cwd: CKEDITOR5_ROOT_DIRECTORY,
stdio: 'inherit'
} );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. squad:platform Issue to be handled by the Platform team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants