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 GitHub Actions builds for oci8 and pdo_oci drivers #4176

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

morozov
Copy link
Member

@morozov morozov commented Jul 21, 2020

Q A
Type improvement
BC Break no

Fixes #4075.

@morozov
Copy link
Member Author

morozov commented Jul 21, 2020

Not sure why code coverage didn't increase but it's not the immediate goal. Once the PR is approved, I'll remove the project from ContinuousPHP.

@morozov morozov marked this pull request as ready for review July 21, 2020 22:25
@morozov morozov added this to the 2.10.3 milestone Jul 21, 2020
@greg0ire
Copy link
Member

greg0ire commented Jul 22, 2020

Not sure why code coverage didn't increase but it's not the immediate goal

There is some insight about this in the output, apparently the file can't be downloaded.:

##[warning]Codecov warning: connect ETIMEDOUT 35.199.43.247:443
bash codecov.sh -n  -F 
codecov.sh: line 1: undefined: command not found
##[warning]Codecov warning: The process 'bash' failed with exit code 127

Consider removing coverage-related parts from this PR and reporting the bug to the action's author.

EDIT: the issue only happens with the PDO_OCI job…

Also, a cool thing to do (possibly in a subsequent PR) would be to leverage job dependencies: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idneeds

@morozov morozov closed this Jul 22, 2020
@morozov morozov reopened this Jul 22, 2020
@morozov
Copy link
Member Author

morozov commented Jul 22, 2020

Consider removing coverage-related parts from this PR and reporting the bug to the action's author.

Why remove it if it doesn't fail the build? Given the error message and the fact that the oci8 build successfully uploaded its report, I'd just consider it a one-off runtime issue.

UPD: after closing and reopening the PR, both coverage reports were successfully submitted.

@greg0ire
Copy link
Member

greg0ire commented Jul 22, 2020

Yeah it does look transient. I see a lot of S in the output, they don't correspond to code that is supposed to run in these jobs do they? Because that would explain the coverage not increasing, and if it does, it would mean we no longer run OCI-related tests.
I'm referring to that kind of thing:

If you have a doubt about that, please commit a failure in such a test, and let's see if the jobs fail?

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

And if you don't have a doubt, just merge

@morozov
Copy link
Member Author

morozov commented Jul 22, 2020

I see a lot of S in the output, they don't correspond to code that is supposed to run in these jobs do they?

This is probably because the PHP runtime doesn't have any DB extensions except for the one tested against. That's why a lot of S.

[…] it would mean we no longer run OCI-related tests.

It would be only possible if the OCI-based extensions weren't installed as claimed by the action but it's not possible because the first commit fixes the failure of the test that only runs if oci8 is installed:

/**
* @requires extension oci8
*/

@greg0ire
Copy link
Member

Ok, maybe CodeCov is lost because it does not have a report for 2.10.x AND oci…

@morozov
Copy link
Member Author

morozov commented Jul 22, 2020

And if you don't have a doubt, just merge

I'm not 100% sure what's going on but I'd rather merge this as is and work on the coverage later. I'd like to get rid of ContinuousPHP and complete all the up-merges first.

@morozov morozov merged commit a6fe19e into doctrine:2.10.x Jul 22, 2020
@morozov morozov deleted the issues/4075 branch July 22, 2020 07:12
@morozov morozov self-assigned this Jul 22, 2020
@morozov
Copy link
Member Author

morozov commented Jul 22, 2020

There's definitely something weird going on beyond our code coverage settings. I just ran the tests locally with the configuration from the repository, and it worked:

$ phpunit -c ci/github/phpunit.oci8.xml --coverage-html $HOME/report

oci8-coverage

@greg0ire
Copy link
Member

greg0ire commented Jul 22, 2020

This url shows that 2 builds were received from GH. When I download them, they contain references to OCI8Exception, such as

<file name="/home/runner/work/dbal/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Exception.php">
        <class name="Doctrine\DBAL\Driver\OCI8\OCI8Exception" namespace="Doctrine\DBAL\Driver\OCI8">
          <metrics complexity="2" methods="1" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="6" coveredstatements="0" elements="7" coveredelements="0"/>
        </class>
        <line num="17" type="method" name="fromErrorInfo" visibility="public" complexity="2" crap="6.00" count="0"/>
        <line num="18" type="stmt" count="0"/>
        <line num="19" type="stmt" count="0"/>
        <line num="20" type="stmt" count="0"/>
        <line num="21" type="stmt" count="0"/>
        <line num="23" type="stmt" count="0"/>
        <line num="24" type="stmt" count="0"/>
        <metrics loc="25" ncloc="17" classes="1" methods="1" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="6" coveredstatements="0" elements="7" coveredelements="0"/>
      </file>

So the issue would be that they are not "merged" with the other ones?

Also, this screen shows only reports from GH, nothing from Travis… but the docs say having multiple sources should not be an issue.

@greg0ire
Copy link
Member

Ah wait, this graph shows a covered OCIException: https://codecov.io/gh/doctrine/dbal/commit/8cce1649b0deea971d8e3730685f36672bbd2145/graphs/sunburst

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code coverage is not collected on Oracle builds
2 participants