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] Implement expected failures file #1036

Merged
merged 5 commits into from
Aug 11, 2020
Merged

[Tests-Only] Implement expected failures file #1036

merged 5 commits into from
Aug 11, 2020

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jul 30, 2020

Implements EXPECTED_FAILURES_FILE the same as owncloud/ocis-reva#402

  1. Adjust .drone.star so that it runs all the core API acceptance tests except ~@notToImplementOnOCIS&&~@toImplementOnOCIS - i.e. except the tests specifically tagged in core PR [Tests-Only] Tag tests to implement on ocis or not owncloud/core#37736

  2. Specify an EXPECTED_FAILURES_FILE and put it in a list of all the acceptance test scenarios that are expected to fail. These are things that pass on oC10 but currently have a problem on cs3org/reva

  3. Add the infrastructure to be able to run local API acceptance test scenarios (Makefile test-acceptance-api target, composer.json, behat.yml,...)

  4. Add local scenarios that demonstrate existing bad behavior in OCIS/reva. These are copied from the scenarios that are currently in core and tagged skipOnOcV10. I have moved just a few of them to demonstrate this. I will move the rest of them in a later PR.

This PR helps with issue owncloud/ocis-reva#282 - when some examples in the Example Table of a Scenario Outline pass and some fail, we will run all of them and the ones that fail can be listed in EXPECTED_FAILURES_FILE

It also removes the need to change core API acceptance test code or feature files as we fix OCIS/reva issues. We can manage the local acceptance tests here in the local repo, and also can manage the list of expected failures here in the local repo (hopefully the list gets shorter as we implement and fix things!)

@phil-davis phil-davis self-assigned this Jul 30, 2020
@phil-davis
Copy link
Contributor Author

local API acceptance tests: https://cloud.drone.io/cs3org/reva/2063/4/7

6 scenarios (6 passed)
37 steps (37 passed)
0m2.48s (18.50Mb)

oC10 API acceptance tests: https://cloud.drone.io/cs3org/reva/2063/4/8

1197 scenarios (534 passed, 663 failed)
10546 steps (8085 passed, 663 failed, 1798 skipped)
16m11.49s (21.43Mb)
Checking expected failures
Success - all failures were expected

Looks good. I will squash the commits.

@phil-davis phil-davis marked this pull request as ready for review July 31, 2020 15:13
@phil-davis phil-davis requested a review from labkode as a code owner July 31, 2020 15:13
@phil-davis
Copy link
Contributor Author

PR #1041 could happen first, to get the core commit id really current. Then I can rebase this PR.

@labkode
Copy link
Member

labkode commented Aug 3, 2020

@phil-davis I've merged #1041

@phil-davis
Copy link
Contributor Author

Rebased and passed:
https://cloud.drone.io/cs3org/reva/2115/4/8

1197 scenarios (534 passed, 663 failed)
10546 steps (8085 passed, 663 failed, 1798 skipped)
17m15.98s (21.46Mb)
Checking expected failures
Success - all failures were expected

@labkode ready for review

@phil-davis
Copy link
Contributor Author

I rebased and force-pushed just now, to check that CI is still all good.

@labkode
Copy link
Member

labkode commented Aug 5, 2020

@phil-davis I am having a hard time to understand this PR as I don't have enough knowledge of the previous ownCloud PHP testing stack. Perhaps we can have a quick call and you guide me through?

@phil-davis
Copy link
Contributor Author

10pm here and I was about to head for bed. I rebased to clean out some commits for the core commit bumps that are now already merged in master.

Last week we implemented a feature in the API test runner code that allows EXPECTED_FAILURES_FILE to be defined. When that is defined, and some test scenarios fail, the fails are checked against the list in that file. This allows us to run a large number of test scenarios from the upstream test suite, and to locally control the list of scenarios that are expected to fail. It is being used in owncloud/ocis-reva now, and a PR is waiting to be merged in owncloud/ocis. The benefit is that we do not need to coordinate tagging in the upstream test suite owncloud/core as fixes are made in cs3org/reva. We can simply reduce the local list of expected failures as we make more test scenarios pass.

A lot of the code here is infrastructure to support the test code. The upstream test code in owncloud/core is written in PHP using Behat+Gherkin as the test runner. So that means having composer.json etc to get the needed test dependencies. These things only get used by the API acceptance tests. Unfortunately I guess they "pollute" this golang repo with some PHP! But it does not find its way into any of the reva build artifacts.

Anyway, llet me know when would be a good time to chat tomorrow or...

@labkode
Copy link
Member

labkode commented Aug 10, 2020

@phil-davis your comment helped understand. Rebase and I'll merge it. Also, there should be a better way to avoid bumping core commit id for the testing suite. Why not relying on master always?

@phil-davis
Copy link
Contributor Author

@labkode rebased

Why not relying on master always?

That might be OK to do now. Previously we relied on the skipOnOcis etc tags in the core API test suite. Some change would be done to those to unskip some test scenarios that now pass and to delete bug-demonstration scenarios that are no longer relevant. But the change/fix happened in cs3org/reva, owncloud/ocis-reva and owncloud/ocis at different times as the fix was merged, a release made, and that release applied to to next upstream repo. So that could be manged by adjusting the core commit id in each repo...

Now that we can control the expected failures locally in each reva repo there should be much less change to the core API test suite, and we can locally adjust to what test scenarios should pass/fail.

After this PR, I can make a PR to remove the core commit id. And we can see how often we get CI broken because of some change in owncloud/core

@phil-davis
Copy link
Contributor Author

Note: I also have #1057 to come after this. It adds more bug-demo scenarios to the local tests, and can split the test pipelines (because the full set of API acceptance tests is getting a bit long!) But I will sort that out after getting this first infrastructure merged.

@phil-davis
Copy link
Contributor Author

@labkode ready for review

@labkode labkode merged commit 7906f54 into cs3org:master Aug 11, 2020
@phil-davis phil-davis deleted the implement-EXPECTED_FAILURES_FILE branch August 11, 2020 07:53
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.

2 participants