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.

Sign up for GitHub

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

Jump to bottom

Add tests for XEP-0045 Multi-User Chat (section 10) #41

Merged
merged 8 commits into from
Sep 21, 2024
Merged

Add tests for XEP-0045 Multi-User Chat (section 10) #41

merged 8 commits into from
Sep 21, 2024

Conversation

guusdk
Copy link
Contributor

@guusdk guusdk commented Aug 15, 2024

This contains all tests for section 10 "Owner Use Cases" of XEP-0045.

Some of the tests included here are completely or partially commented out, as they depend on fixes in the underlying library, Smack, such as:

@guusdk guusdk requested a review from Fishbowler August 15, 2024 19:48
@guusdk guusdk force-pushed the 4h_muc-owner-tests branch 4 times, most recently from 8c2d777 to 14944af Compare August 16, 2024 15:34
@Fishbowler
Copy link
Contributor

What's the solution here? Comment more out to get tests passing in the short term?

@guusdk guusdk force-pushed the 4h_muc-owner-tests branch from 14944af to f6dfe59 Compare August 19, 2024 07:29
@guusdk
Copy link
Contributor Author

guusdk commented Aug 19, 2024

What's the solution here? Comment more out to get tests passing in the short term?

Commenting out tests should only be done if they're not (yet) compatible with either the specification or with the implementation Smack. Openfire not being compliant with the specification should not be a reason to comment out the test (although we may consider not running that particular test for Openfire by modifying our CI setup).

Ideally, we verify that the tests are correct (even if Openfire is not). In parallel, I'm trying to get fixes in Openfire (but those are out of scope of this project).

@guusdk
Copy link
Contributor Author

guusdk commented Aug 19, 2024

I've rebased this PR to the latest head.

@Fishbowler
Copy link
Contributor

Are the remaining failures all Openfire bugs?

@guusdk
Copy link
Contributor Author

guusdk commented Aug 20, 2024

I think so. I've got a rewrite of Openfire in the works on which all of the tests seem to pass.

@guusdk
Copy link
Contributor Author

guusdk commented Aug 22, 2024

The test failures in CI are fixed by igniterealtime/Openfire#2505 and igniterealtime/Openfire#2510 which will go in a future release of Openfire (likely 4.10.0)

@guusdk guusdk force-pushed the 4h_muc-owner-tests branch from aedc7fd to 765c6ad Compare September 2, 2024 18:08
@guusdk
Copy link
Contributor Author

guusdk commented Sep 2, 2024

Rebased.

@guusdk guusdk force-pushed the 4h_muc-owner-tests branch from 765c6ad to a1bdd83 Compare September 5, 2024 18:34
@guusdk
Copy link
Contributor Author

guusdk commented Sep 5, 2024

Rebased, and enabled tests that were commented out pending changes in Smack (that have now landed).

Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

A few comments after reviewing the Admin List tests.

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerAdminListIntegrationTest.java Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerAdminListIntegrationTest.java Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerAdminListIntegrationTest.java Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comments after reviewing the Owner Config tests.

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerConfigRoomIntegrationTest.java Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerConfigRoomIntegrationTest.java Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comment on Create Room tests

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerCreateRoomIntegrationTest.java Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comment on Destroy Room tests

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerDestroyRoomIntegrationTest.java Outdated Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comments on Grant Admin tests

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerGrantAdminIntegrationTest.java Outdated Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerGrantAdminIntegrationTest.java Outdated Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comment on Grant Owner tests

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerGrantOwnerIntegrationTest.java Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

No notes on Chat Owner tests :)

Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comments on Owner List tests

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerOwnerListIntegrationTest.java Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerOwnerListIntegrationTest.java Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerOwnerListIntegrationTest.java Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

Comments on Revoke Admin tests

src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerRevokeAdminIntegrationTest.java Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerRevokeAdminIntegrationTest.java Outdated Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerRevokeAdminIntegrationTest.java Outdated Show resolved Hide resolved
src/main/java/org/jivesoftware/smackx/muc/MultiUserChatOwnerRevokeAdminIntegrationTest.java Show resolved Hide resolved
Fishbowler
Fishbowler reviewed Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

No comments on Revoke Owner tests :)

Fishbowler
Fishbowler approved these changes Sep 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

I've left a lot of comments, but most are prompts or questions. Assuming none of them lead to changes, am definitely happy to merge as-is.

@guusdk
Copy link
Contributor Author

guusdk commented Sep 13, 2024

@Fishbowler thanks for the review! I've pushed a new commit that applies various changes based on your feedback. I have left conversations in this PR unresolved if I'd like to get your feedback on my response. Can you either comment on my comment (on your comment), and/or resolve these please? If everything is resolved, lets hit the merge button!

@guusdk guusdk requested a review from Fishbowler September 13, 2024 11:56
@guusdk
Copy link
Contributor Author

guusdk commented Sep 13, 2024

Lots of test failures in this PR, but when I execute the locally against a version of Openfire that has MUC fixes, they're all green.

@guusdk guusdk mentioned this pull request Sep 18, 2024
Merged
@guusdk
Add tests for XEP-0045 Multi-User Chat (section 10)
965cde7
@guusdk
Improve info to diagnose test failure
10bae5b 
By providing more context for the test, in the form of more quotes from the specification under test, the purpose of a (failed) test is made more clear to end-users.
@guusdk
Separate receiving of result from result content check
f4a9a47 
By separating 'did we recieve X' from 'does X have content Y' the test fails faster when X is received, but does not have content Y.
@guusdk
Shorten MUC room name (to facilitate storage in a database)
583d2c1
@guusdk
Enabled tests that were commented out pending SMACK support
db21a50
@guusdk
Applied review feedback
c92fe9c
@guusdk
Update to Smack 4.5.0-beta2
2c1b1f2 
The new Smack beta has dropped some old API. This commit adjusts our code accordingly.
@guusdk
Copy link
Contributor Author

guusdk commented Sep 18, 2024

I've rebased this PR (and added some changes) to pull in the update to Smack 4.5.0-beta2

@guusdk
CI: Switch to using an daily build of Openfire
e331b9d 
While creating our integration tests, we frequently find bugs in Openfire (which are then quickly resolved, obviously!)

The continuous integration of our tests often fails for tests that identify a bug in Openfire, that has recently been fixed.

To reduce the impact of this problem, CI should use a (very) recent build of Openfire, instead of a fixed version.

Note that the implementation introduced here is not ideal:
- To prevent an excessive amount of downloads, the build caches an Openfire download based on the date that the download was attempted
- The script tries to download todays daily build, but goes back in date (up to 30 days) to find the last available build
- Depending on the time that a daily build is generated and the first execution of this script, the build that is used may very well be an older build than today's.

This can probably be improved upon, but that's beyond  my scripting foo.
@Fishbowler Fishbowler merged commit 6bf62ca into main Sep 21, 2024
7 checks passed
@Fishbowler Fishbowler deleted the 4h_muc-owner-tests branch September 21, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishbowler Fishbowler Fishbowler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@guusdk @Fishbowler