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.

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

Add tests for BSBLAN climate component #124524

Merged
merged 30 commits into from
Sep 8, 2024
Merged

Conversation

liudger
Copy link
Contributor

@liudger liudger commented Aug 24, 2024

Make the test coverage 100%

Breaking change

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/conftest.py Outdated Show resolved Hide resolved
@liudger liudger marked this pull request as ready for review August 26, 2024 08:48
@liudger liudger marked this pull request as draft August 26, 2024 08:49
@liudger
Copy link
Contributor Author

liudger commented Aug 31, 2024

@joostlek to get all attributes tested I need to set some values that bsblan could return depending on the features. Is this the way? a9ec445 It was hard to get it from the other examples how to fake some returns to let the integration return the correct values

@joostlek
Copy link
Member

get all attributes tested

Which ones?
Also there is a merge conflict

@liudger
Copy link
Contributor Author

liudger commented Aug 31, 2024

the current_temperature is None

    # Spoof the current_temperature value to "---"
    mock_bsblan.set_current_temperature("---")

    # Update the state in Home Assistant
    await hass.helpers.entity_component.async_update_entity("climate.bsb_lan")

    # Get the state of the climate entity
    state = hass.states.get("climate.bsb_lan")

    # Assert that the current_temperature attribute is None
    assert state.attributes["current_temperature"] is None

Yeah I will fix the merge conflict. Accidentally fixed a bug twice.

@joostlek
Copy link
Member

But you can repatch the object to return "---" righjt

@liudger liudger marked this pull request as ready for review September 2, 2024 06:34
homeassistant/components/bsblan/diagnostics.py Outdated Show resolved Hide resolved
homeassistant/components/bsblan/entity.py Outdated Show resolved Hide resolved
tests/components/bsblan/conftest.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

home-assistant bot commented Sep 2, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft September 2, 2024 09:36
@liudger liudger marked this pull request as ready for review September 6, 2024 15:01
@home-assistant home-assistant bot requested a review from joostlek September 6, 2024 15:01
@liudger liudger marked this pull request as draft September 6, 2024 15:03
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Oh and they are failing

tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
tests/components/bsblan/test_climate.py Outdated Show resolved Hide resolved
@liudger liudger marked this pull request as ready for review September 6, 2024 20:02
@home-assistant home-assistant bot requested a review from joostlek September 6, 2024 20:02
liudger and others added 24 commits September 7, 2024 14:05
set current_temperature to None in test case. Is this the correct way for testing?
- Update test_async_set_preset_mode to correctly handle ServiceValidationError
- Check for specific translation key instead of full error message
- Ensure consistency between local tests and CI environment
- Import ServiceValidationError explicitly for clarity
This commit refactors the async_set_preset_mode method in the BSBLANClimate class to improve code readability and maintainability. The method now checks if the HVAC mode is not set to AUTO and the preset mode is not NONE before raising a ServiceValidationError.

Co-authored-by: Joost Lekkerkerker <[email protected]>
test_celsius_fahrenheit
test_climate_entity_properties
test_async_set_hvac_mode

test_async_set_preset_mode still broken. Not sure why hvac mode will not set. THis causes error with preset mode set
This commit improves the async_set_temperature method in the BSBLANClimate class. It removes the unnecessary parameter "expected_result" and simplifies the code by directly calling the service to set the temperature. The method now correctly asserts that the thermostat method is called with the correct temperature.
This commit updates the error message in the BSBLANClimate class when trying to set the preset mode.
@joostlek joostlek merged commit bfe19e8 into home-assistant:dev Sep 8, 2024
29 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
@liudger liudger deleted the add-tests branch September 13, 2024 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants