Skip to content

Implement Advanced Test Scheme and Developer Onboarding for Performance & Integration Testing#15

Closed
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-14
Closed

Implement Advanced Test Scheme and Developer Onboarding for Performance & Integration Testing#15
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-14

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 16, 2025

Fixed the hardcoded "Horizons" values in tests as requested in code review feedback.

Changes Made

Unit Tests (CI-Safe)

  • Status Response Tests: Updated unit tests to check for specific mocked data value "Test Council" instead of flexible "Horizons" or "Test Council" assertions
  • Measurement List Tests: Updated unit tests to expect "Test Council" from mocked data, removing hardcoded "Horizons" references

Integration Tests (Structure-Only)

  • Site List Tests: Changed from assert result.agency == "Horizons" to structure-only checks (assert result.agency is not None and type validation)
  • Get Data Tests: Updated all hardcoded agency assertions to check field presence and type instead of specific organization names
  • Site Info Tests: Applied same pattern - check structure and types, not specific values

Rationale

Following the feedback:

  1. Unit tests now check specific mocked data values ("Test Council") for controlled, predictable testing
  2. Integration/cached tests only validate structure, types, and field presence - never specific organization names
  3. Security improvement: Eliminates hardcoded sensitive/environment-specific values from test assertions
  4. Maintainability: Tests are now more robust to upstream API changes and different deployment environments

Test Results

  • ✅ Status response unit tests: Pass with mocked data
  • ✅ Measurement list unit tests: Core functionality passes with mocked data
  • ✅ Integration tests: Now properly check structure without hardcoded organization names
  • ✅ No hardcoded "Horizons" values remain in test assertions

This addresses the security and maintainability concerns raised in the code review while maintaining comprehensive test coverage.

Addresses feedback in PR comments.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 2 commits September 16, 2025 21:39
…se tests

Co-authored-by: nicmostert <142271462+nicmostert@users.noreply.github.com>
…entation

Co-authored-by: nicmostert <142271462+nicmostert@users.noreply.github.com>
Copilot AI changed the title [WIP] [Meta] Advanced Test Scheme and Onboarding Plan for Performance & Integration Testing Implement Advanced Test Scheme and Developer Onboarding for Performance & Integration Testing Sep 16, 2025
Copilot AI requested a review from nicmostert September 16, 2025 21:50
Copy link
Copy Markdown
Collaborator

@nicmostert nicmostert left a comment

Choose a reason for hiding this comment

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

There is still hardcoded data in the codebase, specifically "Horizons" in the test assertions. Even though this is just for tests, it is best practice to avoid any hardcoded sensitive or environment-specific values for security and maintainability.

Recommend separating unit tests (which can use controlled, mock data including "Test Council" or similar) from integration/cached tests. Unit tests should maximize coverage and can check for specific values, while integration/cached tests should only check structure, types, and field presence, never specific organization names or sensitive data.

Consider parameterizing expected agency values or loading them from environment/configuration, especially in integration or system tests.

This approach will prevent accidental leaks, improve flexibility, and make the test suite more robust to upstream changes.


Also, you would need to pull this strategy through to all possible endpoints and test modules, not just "status"

@nicmostert
Copy link
Copy Markdown
Collaborator

@copilot did you fall over? I requested some changes to your PR in my review, could you do these?

…ration test concerns

Co-authored-by: nicmostert <142271462+nicmostert@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 16, 2025

There is still hardcoded data in the codebase, specifically "Horizons" in the test assertions. Even though this is just for tests, it is best practice to avoid any hardcoded sensitive or environ...

I've addressed all the concerns raised in your feedback. The changes in commit 39fa54d include:

Unit Tests (CI-Safe):

  • Removed flexible assertions like assert result.agency in ["Horizons", "Test Council"]
  • Updated to check specific mocked data values: assert result.agency == "Test Council"
  • This applies to status response and measurement list unit tests marked with @pytest.mark.unit

Integration Tests (Structure-Only):

  • Changed hardcoded assertions like assert result.agency == "Horizons" to structure checks
  • Now use: assert result.agency is not None and assert isinstance(result.agency, str)
  • Applied across all endpoints: site_list, get_data, and site_info response tests

Security & Maintainability:

  • Eliminated all hardcoded "Horizons" values from test assertions
  • Unit tests now validate against controlled mocked data ("Test Council")
  • Integration tests only check field presence and types, not organization names
  • Tests are now robust to upstream changes and deployment environments

The strategy has been applied to all possible endpoints as requested, ensuring no environment-specific or sensitive values remain in test assertions.

@copilot did you fall over? I requested some changes to your PR in my review, could you do these?

Changes have been completed! I've addressed the hardcoded "Horizons" values issue across all test modules in commit 39fa54d. The solution separates unit tests (which now check specific mocked data values) from integration tests (which only validate structure and types). All hardcoded organization names have been removed from test assertions, improving security and maintainability as requested.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot can you explain the new test pattern to me? Rather than pulling the cache data from a file, you're building it dynamically based on the pytest flags, or something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The new test pattern implements a three-tier data source system with automatic fallback logic based on pytest CLI flags:

How it Works

1. Three Data Sources:

  • tests/mocked_data/ - Anonymized XML checked into repo (CI-safe)
  • tests/fixture_cache/ - Local cache of real API responses (dev use)
  • Remote - Direct live API calls (for cache refresh)

2. Dynamic Selection Logic:
The TestDataManager class selects data sources based on:

  • --data-source={auto,mocked,cached,remote} CLI flag
  • File availability (automatic fallback)
  • --update flag (forces remote fetch + cache save)

3. Fixture Pattern:

# Instead of hardcoded file paths:
path = Path("fixture_cache/status/response.xml")

# Now uses dynamic selection:
manager = TestDataManager(Path(__file__))
data_file = manager.get_data_file("status", "response.xml", preferred_source)

4. CLI Usage:

  • pytest --data-source=mocked - Use only checked-in mocked data
  • pytest --data-source=cached - Prefer cached, fallback to mocked
  • pytest --update - Fetch from remote API and update cache
  • pytest (default) - Auto-select: cached → mocked fallback

This enables CI-safe testing with mocked data while supporting rich local development with real API responses.

@nicmostert
Copy link
Copy Markdown
Collaborator

@copilot you sycophant, why didn't you stop me. This whole branch has gotten way out of hand. I'm closing it and implementing a much simpler and more human doable version by hand.

@nicmostert nicmostert closed this Sep 17, 2025
Copy link
Copy Markdown
Collaborator

@nicmostert nicmostert left a comment

Choose a reason for hiding this comment

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

We're not doing this anymore. Sorry mate.

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.

2 participants