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-only] [full-ci] Add acceptance tests #1030

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jun 22, 2022

The PHP unit tests were passing both before and after PR #817 - so the unit tests were not effective in finding the problem with code after the Guzzle7 update in core. See issue #1029

To cover the real Guzzle client code that lists available apps and downloads app tarballs from the market-place, we really have to do real requests to the market-place. I looked at the unit tests and it didn't look like it would be easy/possible to mock out stuff while still actually covering the things that might go wrong.

So I have added acceptance tests. The downside is that they require the live production market-place server to be up. So they might sometimes fail if the market-place server is down (for maintenance or broken or...) The tests only run in the CI of this app, so it is not going to be an issue for regular core... CI.

I locally tried reverting the code fix in PR #817 and running the acceptance tests. I get:

  Scenario: install, attempt to reinstall, upgrade and uninstall an app that is available in the market-place # /home/phil/git/owncloud/core/apps-external/market/tests/acceptance/features/cliMain/marketInstallUpgradeUninstall.feature:9
    When the administrator invokes occ command "market:install activity"                                      # OccContext::theAdministratorInvokesOccCommand()
    Then the command should have been successful                                                              # OccContext::theCommandShouldHaveBeenSuccessful()
      The command was not successful, exit code was 1.
      stdOut was: 'activity: Installing new app ...
      activity: Archives of type application/x-empty are not supported
      '
      stdErr was: ''
       (Exception)

So the acceptance tests do cover that problem.

@phil-davis phil-davis self-assigned this Jun 22, 2022
@phil-davis phil-davis force-pushed the add-acceptance-tests branch from 13df8da to 347284c Compare June 22, 2022 10:18
@phil-davis phil-davis force-pushed the add-acceptance-tests branch from 347284c to 63ce9c8 Compare June 22, 2022 11:24
@phil-davis phil-davis marked this pull request as ready for review June 22, 2022 11:24
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@phil-davis
Copy link
Contributor Author

https://drone.owncloud.com/owncloud/market/3215/10/10

7 scenarios (7 passed)

@phil-davis phil-davis requested a review from IljaN June 22, 2022 11:43
@phil-davis
Copy link
Contributor Author

@IljaN the acceptance tests will access the live market-place server when they run for about 3 minutes. That will happen nightly, and whenever someone (or dependabot) makes a PR to this app repo.

I guess that the market-place server gets thousands of requests every day anyway, so the performance difference due to this extra access will not be noticeable.

Does that seem OK to you, and whoever else manages that production server?

@phil-davis phil-davis merged commit a0f6a71 into master Jun 23, 2022
@delete-merged-branch delete-merged-branch bot deleted the add-acceptance-tests branch June 23, 2022 08:00
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.

2 participants