Skip to content

Conversation

@nickvergessen
Copy link
Member

Signed-off-by: Joas Schilling [email protected]

@nickvergessen nickvergessen added this to the 💛 Next Major (24) milestone Apr 5, 2022
@nickvergessen nickvergessen self-assigned this Apr 5, 2022
@nickvergessen nickvergessen force-pushed the techdebt/noid/add-integration-tests-for-notifications branch from 0b2ff07 to 69c1b5e Compare April 5, 2022 12:42
@nickvergessen nickvergessen changed the title Install notifications on drone Add integration tests for notifications Apr 5, 2022
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Rather than installing the notifications app only in Drone I would add:

NOTIFICATIONS_BRANCH="master"

# Add Notifications app to the parent directory of "spreed" (unless it is
# already there or in "apps").
if [ ! -e "${ROOT_DIR}/apps/notifications" ] && [ ! -e "../../../notifications" ]; then
       (cd ../../../ && git clone --depth 1 --branch ${NOTIFICATIONS_BRANCH} https://github.com/nextcloud/notifications)
fi

to the run.sh file to ensure that the notifications app is installed if needed, no matter if the integration tests are run in Drone, by directly calling ./run.sh or in Docker with ./run-docker.sh.

@nickvergessen
Copy link
Member Author

We do it like this with guests app too atm

@nickvergessen nickvergessen requested a review from danxuliu April 6, 2022 07:06
@nickvergessen nickvergessen added 3. to review feature: chat 💬 Chat and system messages feature: api 🛠️ OCS API for conversations, chats and participants technical debt labels Apr 6, 2022
@nickvergessen
Copy link
Member Author

Added installing of both apps in run.sh and added another check for additional app directories

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Other than the comments, 👍

I have added a fixup to remove resetting the current user in user :user has the following notifications, as it is not needed and inconsistent with the rest of steps that get the user as a parameter, and another fixup to simplify cloning the needed apps, as it should not be needed to manually check the paths if app:getpath returned false.

->executeStatement();
} catch (\Throwable $e) {
// Ignore
}
Copy link
Member

Choose a reason for hiding this comment

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

The other queries are not guarded against exceptions, should they be guarded too or they will always succeed and this one has something special that could cause the query to throw in some cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is guarded as it is against the table of another app and I thought better save then annoyed

Comment on lines +8 to +11
When user "participant1" creates room "one-to-one room" (v4)
| roomType | 1 |
| invite | participant2 |
Given user "participant2" joins room "one-to-one room" with 200 (v4)
Copy link
Member

Choose a reason for hiding this comment

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

I would say that everything before sending the message should be a precondition, and therefore the steps should be prefixed by Given and And (here and in all the scenarios of this file), but I guess that is just a matter of taste :-)

@nickvergessen nickvergessen force-pushed the techdebt/noid/add-integration-tests-for-notifications branch from 4c6e715 to 65ae297 Compare April 6, 2022 12:16
@nickvergessen nickvergessen merged commit f7505e0 into master Apr 6, 2022
@nickvergessen nickvergessen deleted the techdebt/noid/add-integration-tests-for-notifications branch April 6, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review feature: api 🛠️ OCS API for conversations, chats and participants feature: chat 💬 Chat and system messages technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants