Add some basic integration tests #100
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the GitHub Actions workflow to include submodule support, the addition of a new submodule in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lawrence-forooghian
force-pushed
the
fix-Message-createdAt-decoding
branch
from
77456aa to
99136ef
Compare
lawrence-forooghian
force-pushed
the
basic-integration-tests
branch
from
a97455b to
e136ae4
Compare
lawrence-forooghian
force-pushed
the
fix-Message-createdAt-decoding
branch
from
99136ef to
9454e40
Compare
lawrence-forooghian
force-pushed
the
basic-integration-tests
branch
3 times, most recently
from
2681b87 to
67d4145
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
Tests/AblyChatTests/IntegrationTests.swift (2)
5-9: Consider increasing the time limit and adding setup documentation.While one minute might be sufficient for basic tests, integration tests can be sensitive to network conditions and API response times. Additionally, the documentation could be enhanced to include:
- Prerequisites (e.g., sandbox environment setup)
- Test data requirements
- Expected network conditions
@Suite( - .timeLimit(.minutes(1)) + .timeLimit(.minutes(2)) ) -/// Some very basic integration tests, just to check that things are kind of working. +/// Integration tests validating core chat functionality: +/// - Message sending and receiving +/// - Historical message retrieval +/// - Room attachment/detachment +/// +/// Prerequisites: +/// - Sandbox environment access +/// - Network connectivity +/// Note: Tests may take longer under slow network conditions struct IntegrationTests {
1-58: Consider test organization strategies.As mentioned in the PR objectives, separating integration tests from unit tests would be beneficial. Consider these approaches:
- Use test tags to filter integration tests:
@Suite("integration")
- Move integration tests to a separate target in Package.swift:
.testTarget( name: "AblyChatIntegrationTests", dependencies: ["AblyChat"] )This would allow running integration tests separately using:
swift test --filter AblyChatIntegrationTestsCONTRIBUTING.md (2)
11-13: Consider documenting the purpose of the submodule.While the setup instructions are clear, it would be helpful to briefly explain what the submodule contains and why it's necessary for development. This context would help new contributors understand the project structure better.
-1. `git submodule update --init` +1. `git submodule update --init` # Initialize the ably-common submodule containing shared test utilities
29-29: Enhance integration testing guidelines.While the guideline to add integration tests is good, consider expanding it to include:
- What constitutes "core functionality" that should be smoke tested
- Best practices for writing integration tests
- Guidelines for managing test execution time (as mentioned in PR objectives about separating integration tests from unit tests)
Example addition:
-If you add a new feature, try to extend the `IntegrationTests` tests to perform a smoke test of its core functionality. +If you add a new feature, try to extend the `IntegrationTests` tests to perform a smoke test of its core functionality. Core functionality typically includes: +- Basic happy path scenarios +- Critical error cases +- Integration points with Ably's core services + +To manage test execution time: +- Keep integration tests focused on end-to-end workflows +- Use unit tests for detailed edge cases +- Tag tests with `@IntegrationTest` to allow separate executionTests/AblyChatTests/Helpers/Sandbox.swift (2)
31-31: Avoid force unwrapping URL stringsForce unwrapping the result of
URL(string:)can lead to a crash if the URL is invalid. Even though the URL is hardcoded, it's good practice to handle it safely.Apply this diff:
- var request = URLRequest(url: .init(string: "https://sandbox-rest.ably.io/apps")!) + guard let url = URL(string: "https://sandbox-rest.ably.io/apps") else { + throw Error.invalidURL + } + var request = URLRequest(url: url)Add to your
Errorenum:case invalidURL
17-29: Consider refactoringloadAppCreationRequestBody()to improve readability and efficiencyThe current implementation involves reading a JSON file, deserializing it into a dictionary, extracting a subcomponent, and then serializing it back to
Data. This can be streamlined by decoding directly into a Codable struct.Here's how you might refactor the method:
private struct AppSetup: Codable { var post_apps: [PostApp] } private struct PostApp: Codable { // Define the properties as per your JSON structure } private static func loadAppCreationRequestBody() throws -> Data { guard let testAppSetupFileURL = Bundle.module.url( forResource: "test-app-setup", withExtension: "json", subdirectory: "ably-common/test-resources" ) else { throw Error.resourceNotFound("test-app-setup.json") } let data = try Data(contentsOf: testAppSetupFileURL) let appSetup = try JSONDecoder().decode(AppSetup.self, from: data) return try JSONEncoder().encode(appSetup.post_apps) }This approach eliminates the need for force casting and makes the code more type-safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/check.yaml(6 hunks).gitmodules(1 hunks).prettierignore(1 hunks)CONTRIBUTING.md(2 hunks)Package.swift(1 hunks)Tests/AblyChatTests/Helpers/Sandbox.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/ably-common(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .gitmodules
- .prettierignore
- Tests/AblyChatTests/ably-common
🔇 Additional comments (3)
Package.swift (2)
Line range hint 19-24: Consider switching ably-cocoa to a tagged version before integration tests.
The TODO comment indicates using a branch instead of a tag. For integration tests, it's particularly important to pin dependencies to specific versions to ensure test stability and reproducibility.
Let's check the available tags for ably-cocoa:
#!/bin/bash
# Description: Check available stable tags for ably-cocoa
echo "Fetching latest tags from ably-cocoa repository:"
gh api repos/ably/ably-cocoa/tags --jq '.[].name' | grep -v beta | grep -v alpha | head -n 5Consider prioritizing issue #80 to switch to a stable tagged version before implementing the integration test suite.
61-63: LGTM! Verify resource path matches submodule.
The addition of the ably-common resource to the test target is correct and aligns with the integration testing objectives.
Let's verify the resource path matches the submodule location:
✅ Verification successful
Resource path correctly matches submodule location ✓
The verification confirms that:
- The submodule is correctly configured at
Tests/AblyChatTests/ably-common - The directory exists at the expected location
- The resource path in Package.swift is correctly referenced relative to the test target
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ably-common submodule location matches the resource path
# Test: Check if the submodule exists at the expected path
if [ -f ".gitmodules" ]; then
echo "Checking .gitmodules content:"
cat .gitmodules
fi
# Test: Verify the directory structure
echo -e "\nChecking directory structure:"
ls -la Tests/AblyChatTests/ably-common 2>/dev/null || echo "Directory not found at expected path"
Length of output: 334
Script:
#!/bin/bash
# Let's try a different approach to verify the submodule and resource path
# Check if .gitmodules exists and its contents
if [ -f ".gitmodules" ]; then
echo "=== .gitmodules content ==="
cat .gitmodules
else
echo ".gitmodules file not found"
fi
echo -e "\n=== Directory structure ==="
# List contents of the root directory to see ably-common
ls -la | grep "ably-common"
# Try to find ably-common directory anywhere in the repository
echo -e "\n=== Finding ably-common directory ==="
fd "ably-common" --type d
# Check Package.swift content around the resource declaration
echo -e "\n=== Package.swift context ==="
rg -C 5 "\.copy\(\"ably-common\"\)" Package.swift
Length of output: 917
.github/workflows/check.yaml (1)
20-21: LGTM! Consistent submodule support added across all jobs.
The addition of submodules: true to all actions/checkout@v4 steps is correct and necessary to support the new integration tests that depend on the ably-common submodule.
Let's verify that all checkout actions have been updated:
Also applies to: 47-48, 66-67, 87-88, 108-109, 128-129
✅ Verification successful
Let me gather more context to verify if these checkout actions are properly configured with submodules support.
All checkout actions correctly configured with submodule support
The verification confirms that all six instances of actions/checkout@v4 in the workflow file are properly configured with submodules: true. This is consistent with the changes needed to support the new integration tests that depend on the ably-common submodule.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all checkout actions include submodule support
# Expected: All checkout actions should have the submodules parameter
# Search for checkout actions without submodule support
echo "Checking for any checkout actions without submodule support..."
rg "uses: actions/checkout@v4\s*$" .github/workflows/check.yaml
# If any are found, they will be listed above. No output means all are properly configured.
Length of output: 404
Script:
#!/bin/bash
# Get more context around the checkout actions to see their complete configuration
rg -A 2 "uses: actions/checkout@v4" .github/workflows/check.yaml
Length of output: 531
lawrence-forooghian
force-pushed
the
basic-integration-tests
branch
2 times, most recently
from
9c8d181 to
6f69d1d
Compare
lawrence-forooghian
requested review from
maratal and
umair-ably
and removed request for
maratal and
umair-ably
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Tests/AblyChatTests/IntegrationTests.swift (1)
7-7: Offer assistance with implementing a timeout functionNoticed that you considered adding a timeout to the tests but deferred due to compatibility concerns. If you'd like, I can help provide a custom timeout implementation that's compatible with earlier iOS versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/check.yaml(6 hunks).gitmodules(1 hunks).prettierignore(1 hunks)CONTRIBUTING.md(2 hunks)Package.swift(1 hunks)Tests/AblyChatTests/Helpers/Sandbox.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/ably-common(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/check.yaml
- .gitmodules
- .prettierignore
- CONTRIBUTING.md
- Package.swift
- Tests/AblyChatTests/Helpers/Sandbox.swift
- Tests/AblyChatTests/ably-common
🔇 Additional comments (2)
Tests/AblyChatTests/IntegrationTests.swift (2)
28-34: Consider using dynamic room IDs to avoid test interference
Using a static room ID like "basketball" may cause test interference when running tests concurrently or multiple times. Generating a unique room ID for each test can prevent potential conflicts.
24-68: Add resource cleanup to prevent potential leaks
The test creates chat clients but doesn't clean them up after execution. This could lead to resource leaks in the testing environment. Consider adding cleanup code to close clients and release resources after the test completes.
|
Something is intermittently failing; will investigate. Putting back to draft. |
lawrence-forooghian
force-pushed
the
basic-integration-tests
branch
from
6f69d1d to
b60c81c
Compare
It’s unlikely that we’re going to have a working unified test suite before the beta release, so here are some very basic smoke tests just to give us a _little bit_ of confidence that things are kind of working and that we don’t introduce major regressions. Would be good to have a way of separating these from the unit tests so that they don’t slow them down, but can figure that out later; I don’t have loads of time to spend on this at the moment.
lawrence-forooghian
force-pushed
the
basic-integration-tests
branch
from
b60c81c to
1c2bf47
Compare
|
OK, ready now. |
It’s unlikely that we’re going to have a working unified test suite before the beta release, so here are some very basic smoke tests just to give us a little bit of confidence that things are kind of working and that we don’t introduce major regressions.
Would be good to have a way of separating these from the unit tests so that they don’t slow them down, but can figure that out later; I don’t have loads of time to spend on this at the moment.
Summary by CodeRabbit
Release Notes
New Features
Sandboxenum for creating API keys in the Ably sandbox environment.Documentation
CONTRIBUTING.mdwith new setup instructions and testing guidelines..prettierignoreto exclude specific test directories from formatting.Chores