Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Sep 24, 2025

Working on issue #16: Add test to check alarm timestamp when it turns inactive ✅

Completed:

  • Understand the issue requirements: Need to add test that verifies alarm timestamps are different when alarm turns active vs inactive
  • Explore existing test structure and patterns in spec/site/tlc/alarm_spec.rb
  • Verify current tests run with dry-run to understand the setup
  • Identify with_alarm_activated helper method usage pattern
  • Add new test that captures timestamps for both active and inactive alarm states
  • Follow existing patterns for test structure, naming, and documentation
  • Address core version requirements: Tag test with core: '>=3.1.4' for the new behavior
  • Add pre-3.1.4 test: Added separate test for earlier core versions where behavior differs
  • Refine based on feedback: Use consistent terminology, remove unnecessary checks, simplify assertions
  • Verify the new tests run successfully and are properly structured
  • Ensure changes are minimal and focused

Summary:

Added comprehensive test coverage for alarm timestamp behavior across RSMP core versions:

For RSMP core >= 3.1.4 (core: '>=3.1.4'):

  • Test: 'A0302 has different timestamps for active and inactive states'
  • Behavior: Verifies inactive timestamp is after active timestamp (simplified from checking both difference and sequence)
  • Rationale: RSMP core >= 3.1.4 changed behavior so alarm timestamps represent when status changes

For RSMP core < 3.1.4 (core: '<3.1.4'):

  • Test: 'A0302 alarm timestamps in RSMP core < 3.1.4 versions'
  • Behavior: Verifies both active/inactive states are received without timestamp requirements
  • Rationale: Earlier versions only tracked when alarm became active, not status changes

Refinements based on feedback:

  • Updated terminology to use "RSMP core >= 3.1.4" and "RSMP core < 3.1.4" for consistency
  • Removed checks that timestamps are close to now (unnecessary timing validation)
  • Simplified core >= 3.1.4 test to only check inactive > active (removed redundant difference check)
  • Removed unnecessary logging statements and success messages for cleaner output

The implementation properly handles the core version differences as requested in the review feedback, ensuring tests run appropriately based on the RSMP core version being tested.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Work on #16 by adding one or more RSpec tests in spec/site/tlc/alarm_spec.rb. Follow existing patterns for how an alarm is triggered by activating a specific input, using the helper method with_alarm_activated(). Follow existing patterns for file place... Add test for alarm timestamp differences between active and inactive states Sep 24, 2025
Copilot AI requested a review from emiltin September 24, 2025 13:08
Copilot finished work on behalf of emiltin September 24, 2025 13:08
@emiltin emiltin added the testhub Set this label to trigger test on test hub equipment label Sep 26, 2025
@emiltin emiltin marked this pull request as ready for review September 30, 2025 06:34
Copy link
Member

@emiltin emiltin left a comment

Choose a reason for hiding this comment

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

@copilot please see my review, we need to consider different core versions


# Validate that alarm timestamps are different when alarm turns active vs inactive.
#
# This test verifies RSMP 3.1.4 behavior where the alarm timestamp (aTs)
Copy link
Member

Choose a reason for hiding this comment

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

if this behaviour differs between core versions, we need to tag the test with the appropriate core version, and maybe have more than one test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added core version tagging in commit 5c2bfbd:

  1. RSMP 3.1.4+: Test tagged with core: '>=3.1.4' - verifies timestamps differ between active/inactive states (new behavior)
  2. Pre-3.1.4: Added separate test with core: '<3.1.4' - verifies both states received without requiring different timestamps (legacy behavior)

The tests now properly handle the core version differences where alarm timestamp behavior changed in RSMP 3.1.4.

Copy link
Contributor Author

Copilot AI commented Sep 30, 2025

@copilot please see my review, we need to consider different core versions

I've addressed the core version requirements in commit 5c2bfbd. The test now has proper core version tagging:

  • Main test tagged with core: '>=3.1.4' for the new behavior where timestamps differ between active/inactive states
  • Added separate test with core: '<3.1.4' for pre-3.1.4 versions where behavior differs

This ensures tests run appropriately based on the RSMP core version being tested.

Copilot AI requested a review from emiltin September 30, 2025 06:42
Copilot finished work on behalf of emiltin September 30, 2025 06:42
@emiltin
Copy link
Member

emiltin commented Sep 30, 2025

@copilot
instead of "RSMP 3.1.4+" i would prefer "RSMP core >= 3.1.4", since ">=3.1.4" is what used in the tags, and elsewhere in the docs. instead of "pre-RSMP 3.1.4" or "Pre-3.1.4", use "RSMP core < 3.1.4". note the it's good to use "core", since otherwise it's not clear if version refer to core or the sxl.

to focus, let's remove checks that timestamps are close to now.

in the core >= 3.1.4 test, instead of checking that timestamps a!=b && a>b, we cna just to a>b, so remove the check that they differ.

We don't need log("Testing alarm #{alarm_code_id} timestamp behavior in pre-3.1.4 versions"), as this is described by the test name and tags, and will already to logged automatically. Also remove 'success' logging like log("Verified alarm #{alarm_code_id} has different timestamps: Active=#{active_timestamp}, Inactive=#{inactive_timestamp}")

…ersion terminology, remove timing checks, simplify assertions

Co-authored-by: emiltin <[email protected]>
Copy link
Contributor Author

Copilot AI commented Sep 30, 2025

@copilot
instead of "RSMP 3.1.4+" i would prefer "RSMP core >= 3.1.4", since ">=3.1.4" is what used in the tags, and elsewhere in the docs. instead of "pre-RSMP 3.1.4" or "Pre-3.1.4", use "RSMP core...

Updated the tests in commit 0c2c1dc based on your feedback:

  • Changed "RSMP 3.1.4+" to "RSMP core >= 3.1.4" and "pre-RSMP 3.1.4" to "RSMP core < 3.1.4" for consistency
  • Removed all timestamp proximity checks (be_within(1.minute))
  • Simplified core >= 3.1.4 test to only check inactive_timestamp > active_timestamp (removed redundant difference check)
  • Removed unnecessary logging like "Testing alarm..." and success messages

Copilot finished work on behalf of emiltin September 30, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testhub Set this label to trigger test on test hub equipment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants