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

Tests should crash on non zero exit code #7342

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

mamazu
Copy link
Contributor

@mamazu mamazu commented Apr 3, 2024

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets -
Related issues/PRs #6834
License MIT
Documentation PR -

What's in this PR?

This adds the function that the tests crash when one of the steps exists with a non-zero exit code.

Why?

Currently the tests just ignore the exit code and continue running which then causes issues further down the suite. This should make the tests easier to debug as the errors are reported earlier and closer to the source.

@alexander-schranz
Copy link
Member

alexander-schranz commented Apr 3, 2024

Currently the tests just ignore the exit code and continue running which then causes issues further down the suite.

This is the expected behaviour of the runtests script. We run always all scripts and not exit directly when there is an error in one of the scripts. Else we don't have all errors in the CI for all bundles.

@mamazu
Copy link
Contributor Author

mamazu commented Apr 3, 2024

Okay the problem I was encountering in this case is the test shouldn't run if the init step fails (see the jackalope 2 merge request).

@mamazu
Copy link
Contributor Author

mamazu commented Apr 3, 2024

And the test cases in this case aren't affected because they are allowed to fail, their failure is recorded and just passed on.

@alexander-schranz
Copy link
Member

I see I would update the init_dbal part to throw exception there if the process for schema update finish with a none zero exit code.

@alexander-schranz
Copy link
Member

alexander-schranz commented Apr 3, 2024

Something like:

    $process = exec_sf_cmd(
        'doctrine:schema:update --force'
    );
    
    if (0 !== $process->getExitCode()) {
        // error already outputted before via write_error

        exit($process->getExitCode());
    }

@mamazu
Copy link
Contributor Author

mamazu commented Apr 3, 2024

That's also acceptable.

@alexander-schranz
Copy link
Member

We should target the 2.4 for this change.

@mamazu mamazu changed the base branch from 2.6 to 2.4 April 3, 2024 15:51
@mamazu
Copy link
Contributor Author

mamazu commented Apr 3, 2024

Changed the base.

bin/runtests Outdated
));
$process = exec_sf_cmd('doctrine:schema:update --force');
if ($process->getExitCode() !== 0) {
exit($process->getExitCode());
Copy link
Member

Choose a reason for hiding this comment

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

we should not exit when one bundle fails to init.

the other bundle tests should still be executed.

exec_cmd(sprintf(
PHP_BINARY . ' %s %s',
$console,
' sulu:document:initialize --purge --force --ansi'
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to catch this. Aslong as the tests run as expected it is not required that a bundle uses the document manager bundle.

@mamazu
Copy link
Contributor Author

mamazu commented Apr 3, 2024

I wanted to catch this: https://github.com/sulu/sulu/actions/runs/8525431456/job/23352362111?pr=6834

The Init Task is successful although the update of the schema failed. Without this none of the tests will work.

bin/runtests Outdated Show resolved Hide resolved
bin/runtests Outdated Show resolved Hide resolved
@alexander-schranz
Copy link
Member

That error would still be catched we just don't should exit on the init_bundle part.

@alexander-schranz alexander-schranz added the Bug Error or unexpected behavior of already existing functionality label Apr 10, 2024
@mamazu
Copy link
Contributor Author

mamazu commented Apr 10, 2024

Looks good can you apply those and merge because the app doesn't support that.

@alexander-schranz alexander-schranz enabled auto-merge (squash) April 10, 2024 10:01
@alexander-schranz alexander-schranz merged commit 95c340b into sulu:2.4 Apr 10, 2024
7 of 8 checks passed
@mamazu mamazu deleted the exit-code-crash branch April 24, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error or unexpected behavior of already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants