Skip to content

Conversation

@jguz-pubnub
Copy link
Contributor

No description provided.

@pubnub-ops-terraform
Copy link

pubnub-ops-terraform commented May 27, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

It looks good. I've left few notes about tests, where we removing / adding something and actually don't verify that operation actually performed this modification (not just success status).

final class ChannelGroupEndpointIntegrationTests: XCTestCase {
let config = PubNubConfiguration(from: Bundle(for: ChannelGroupEndpointIntegrationTests.self))

func testListChannelGroups() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is a bit fragile because depends on the current situation with keys, where bunch of channel groups has been created. There is a chance that they will be wiped one day and this test will make false result.

Or it will be fine even if there is just empty array? Should we create in this test at least one channel group which will be pulled and then at the end remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's remove this test according to the docs statement at the top:

https://www.pubnub.com/docs/sdks/rest-api/get-all-channel-groups

Comment on lines 84 to 85
XCTAssertEqual(response.group, testGroup)
XCTAssertEqual(Set(response.channels), Set(testChannels))
Copy link
Contributor

Choose a reason for hiding this comment

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

This information added by SDK itself and not from response? I tried to make a request to self-check and there is no information about group and added channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this information is added by the SDK itself:

https://github.com/pubnub/swift/blob/refactor/integration-tests/Sources/PubNub/PubNub.swift#L915

On a successful response, it returns the list of channels and channel groups that were passed as arguments

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I hope it won't hit us back that users will miss same behaviour in other languages :D

wait(for: [listChannelsExpect], timeout: 10.0)
}

func testAddChannelsToGroup() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we actually validate that channels has been added to the channel group (using listChannels)?

wait(for: [addChannelsExpect], timeout: 10.0)
}

func testRemoveChannelsFromGroup() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we actually validate that channels has been removed from the channel group (using listChannels)?

wait(for: [removeChannelsExpect], timeout: 10.0)
}

func testRemoveChannelGroup() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we actually validate that channel groups has been removed (using listChannelGroups)?

client.allChannelMetadata(sort: [.init(property: .name)]) { result in
let expectedChannels = setupTestChannels(client: client)

client.allChannelMetadata(filter: "id LIKE 'swift-*'") { result in
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice solution with filtering.
Same concerns about how it could be fragile without pre-populated data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, there's no other choice than pre-populating channel/user metadata first before testing whether fetching all of them works as expected

@jguz-pubnub jguz-pubnub merged commit 9f811ed into master Jun 5, 2025
42 of 50 checks passed
@jguz-pubnub jguz-pubnub deleted the refactor/integration-tests branch June 5, 2025 13:04
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.

3 participants