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] copy test dependencies from ownCloud/core (stable-2.0) #5306

Merged
merged 11 commits into from
Jan 5, 2023

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 29, 2022

Description

Copy test infrastructure from the oC/core repository

This is a port of #5280 to the stable-2.0 branch. See discussion in that PR and in the issue for explanation of what is being done.

The API tests no longer use anything from owncloud/core

Related Issue

#5094

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@phil-davis
Copy link
Contributor Author

Note: we will leave this here until next week. Also leave #5280
I will switch it to draft for now.

I would like to double-check that all other test changes have been ported the same into both master and stable-2.0 branches, and get any recent test code changes that have happened in core. And then merge when we are happy that everything is "in sync".

@phil-davis phil-davis marked this pull request as draft December 30, 2022 06:30
@saw-jan saw-jan force-pushed the remove-ownCloud-core-test-dependency-stable branch 3 times, most recently from e5076de to 06d7c51 Compare January 3, 2023 06:48
@saw-jan saw-jan self-assigned this Jan 3, 2023
@saw-jan
Copy link
Member

saw-jan commented Jan 3, 2023

Todos:

  • update expected failure list

@saw-jan saw-jan force-pushed the remove-ownCloud-core-test-dependency-stable branch 8 times, most recently from 3fb009d to a2dfecf Compare January 3, 2023 11:43
kiranparajuli589 and others added 8 commits January 4, 2023 09:28
Signed-off-by: Kiran Parajuli <kiranparajuli589@gmail.com>
…ests run

Signed-off-by: Kiran Parajuli <kiranparajuli589@gmail.com>
Signed-off-by: Kiran Parajuli <kiranparajuli589@gmail.com>
remove unnecessary script from run.sh
remove oc10 specific test suites

provide behat config with make command

fix typo

add missing helpers
@saw-jan saw-jan force-pushed the remove-ownCloud-core-test-dependency-stable branch from ce314a1 to 998da12 Compare January 4, 2023 03:54
@saw-jan
Copy link
Member

saw-jan commented Jan 4, 2023

Changes to notice:

  • feature suites are prefixed core and copied to the tests/acceptance/features folder (e.g. ApiMain -> coreApiMain)
  • separate behat config for copied core tests (tests/acceptance/config/behat-core.yml)
  • core API tests run in parts in CI

These structures can be discussed.

CC @kiranparajuli589 @phil-davis @individual-it

Copy link
Contributor Author

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Looking good to me.

Comment on lines +511 to +517
if [ "${TEST_OCIS}" != "true" ] && [ "${TEST_REVA}" != "true" ]
then
# We are testing on an ownCloud core server.
# Tell the tests to wait 1 second between each upload/delete action
# to avoid problems with actions that depend on timestamps in seconds.
export UPLOAD_DELETE_WAIT_TIME=1
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bit of code might now be useless here in ocis.
But maybe it will be needed (and the if condition modified a bit) if we enable the parallel deployment tests.
So we can leave it here for now.

Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

I think the structure is fine for now.
From the .drone.star file we should be to delete the functions

  • cacheCoreReposForTesting
  • cloneCoreRepos

@phil-davis
Copy link
Contributor Author

I think that these cleanup things can be done in a next PR after we merge this one. For future comparisons, it is probably cleaner to merge all the feature files as direct copies from core. Then delete unneeded stuff separately. That way the deletions will appear in diff listings that can be easily seen.

  • remove any scenarios that are tagged skipOnOcis

The core API tests run with these filter tags:

~@skipOnGraph&&~@skipOnOcis&&~@notToImplementOnOCIS&&~@toImplementOnOCIS&&~comments-app-required&&~@federation-app-required&&~@notifications-app-required&&~systemtags-app-required&&~@local_storage&&~@skipOnOcis-OCIS-Storage&&~@caldav&&~@carddav&&@api&&~@skip
  • look through the filter tags and sort out which ones we still care about.
  • remove scenarios for any tags that we know are never gong to be relevant to ocis or reva
  • cut-down the filter-tags list

@phil-davis
Copy link
Contributor Author

The core-api CI pipelines look good - they run suites and scenarios without ever fetching anything from the owncloud/core repo.

@saw-jan
Copy link
Member

saw-jan commented Jan 4, 2023

This PR is ready from my side.

@phil-davis
Copy link
Contributor Author

Note: these test suites have not been copied from core:
apiComments - comments will not work like that in ocis anyway, if and when implemented
apiFederation* - "core federation" is an oC10 thing
apiProvisioning-v* - we don't use the provisioning API any more
apiProvisioningGroups-v* - we don't use the provisioning API any more
apiShareToRoot - these are for shares received directly in the sharees root folder. That does not happen in ocis
apiSharingNotifications* - they are specific to the notifications app behavior in oC10
apiTags - that is a specific implementation of tags for oC10

That looks good to me.

@phil-davis
Copy link
Contributor Author

I think the structure is fine for now. From the .drone.star file we should be to delete the functions

* cacheCoreReposForTesting

* cloneCoreRepos

Those are now only used by parallelDeployAcceptancePipeline - we could remove them now, or we leave them as-is and sort out the needed refactoring when the parallelApiTests are enabled again "some day".

@phil-davis phil-davis marked this pull request as ready for review January 4, 2023 08:39
@individual-it
Copy link
Member

as discussed in the standup meeting, parallel deployment tests are not prio now, so lets delete cacheCoreReposForTesting & cloneCoreRepo now and the parallel deployment tests in an other PR as its dead code. We can bring it back when its needed.

@phil-davis
Copy link
Contributor Author

as discussed in the standup meeting, parallel deployment tests are not prio now, so lets delete cacheCoreReposForTesting & cloneCoreRepo now and the parallel deployment tests in an other PR as its dead code. We can bring it back when its needed.

I will push another commit to cleanup.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 4, 2023

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@phil-davis
Copy link
Contributor Author

@individual-it @micbar this is ready for review. We need 2 approvals to merge. I have reviewed the code that was done by @kiranparajuli589 and @saw-jan but I cannot approve because I created the PR.

There are many thousands of lines because lots of code and feature files have been copied from owncloud/core - I don't expect all the lines to be individually reviewed!

@ScharfViktor
Copy link
Contributor

looks cool
questions: should we clean up some of the moved tests? for example: tagsTests or tets with @skipOnOcis tag
Or will we clean it up later in another PR?

@phil-davis
Copy link
Contributor Author

Or will we clean it up later in another PR?

I suggest that we clean up in another PR - see my comment #5306 (comment)

@ScharfViktor
Copy link
Contributor

ScharfViktor commented Jan 4, 2023

yes, sorry. I saw
then in order not to lose what i found. need remove:

  • TagsContext
  • TagsHelper
  • TranslationHelper
  • translation.feature

@phil-davis phil-davis merged commit bf95981 into stable-2.0 Jan 5, 2023
@delete-merged-branch delete-merged-branch bot deleted the remove-ownCloud-core-test-dependency-stable branch January 5, 2023 03:37
ownclouders pushed a commit that referenced this pull request Jan 5, 2023
Merge: 13a7065 b1c0ea6
Author: Phil Davis <phil@jankaritech.com>
Date:   Thu Jan 5 09:22:18 2023 +0545

    Merge pull request #5306 from owncloud/remove-ownCloud-core-test-dependency-stable

    [tests-only][full-ci] copy test dependencies from ownCloud/core (stable-2.0)
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.

5 participants