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

available versions list unified by consider prereleases and core compatible in same function #368

Merged
merged 13 commits into from
Mar 15, 2023

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Mar 9, 2023

fixes #360

The available_versions of _AiidalabApp data class object not considered core compatible. The core compatible check is implement independently and sporadically when the available_versions list is required.
This commit add the core compatible check inside the function available_versions of _AiidalabApp class to get the only usable versions for both when prereleases option is on and off. Therefore the version list conforms with what is shown in the version_to_install dropdown option.

@unkcpz unkcpz marked this pull request as draft March 9, 2023 22:42
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@6acf490). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #368   +/-   ##
=======================================
  Coverage        ?   59.36%           
=======================================
  Files           ?       12           
  Lines           ?     1110           
  Branches        ?        0           
=======================================
  Hits            ?      659           
  Misses          ?      451           
  Partials        ?        0           
Flag Coverage Δ
py-3.10 59.36% <0.00%> (?)
py-3.8 59.36% <0.00%> (?)
py-3.9 59.36% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@unkcpz unkcpz changed the title WIP: available versions list unified available versions list unified by consider prereleases and core compatible in same function Mar 9, 2023
@unkcpz unkcpz marked this pull request as ready for review March 9, 2023 22:58
@unkcpz unkcpz requested a review from danielhollas March 9, 2023 22:58
@unkcpz
Copy link
Member Author

unkcpz commented Mar 10, 2023

I test this in the old stack, all good for me. The update button is not wrongly activated anymore.

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

@unkcpz thanks so much for fixing this and adding a test. I also tested both in the new and old Docker stack.

My only request, while we are here, could you please add a bunch more tests for testing different scenarios for the AppRemoteUpdateStatus:

  1. App is not registered. The AppRemoteUpdateStatus should be set accordingly.
  2. App has a compatible update.
  3. App is at the latest version.

aiidalab/app.py Show resolved Hide resolved
tests/test_appdataclass.py Outdated Show resolved Hide resolved
@danielhollas danielhollas added the bug Something isn't working label Mar 14, 2023
@unkcpz unkcpz force-pushed the fix/360/update-status-version-issue branch from 2df9d9e to 1be329c Compare March 14, 2023 10:11
@unkcpz unkcpz force-pushed the fix/360/update-status-version-issue branch from 1be329c to f644388 Compare March 14, 2023 10:52
@unkcpz
Copy link
Member Author

unkcpz commented Mar 14, 2023

  • App is not registered. The AppRemoteUpdateStatus should be set accordingly.
  • App has a compatible update.
  • App is at the latest version.

The first is covered by test_app_is_not_registered and test_update_status_of_unregistred_app, 2 and 3 are covered by test_update_status_latest_version_incompatible already.

aiidalab/app.py Outdated Show resolved Hide resolved
@danielhollas
Copy link
Contributor

@unkcpz thanks! I will review and test again this evening.

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

@unkcpz just a one ask to change the app path in the fixture.

test_update_status_of_unregistred_app, 2 and 3 are covered by test_update_status_latest_version_incompatible already.

I don't see the case where an app is installed and has a valid update available so that its status is AppRemoteUpdateStatus.UPDATE_AVAILABLE. I guess just adding an assert for this to some existing test would suffice?

aiidalab/__main__.py Outdated Show resolved Hide resolved
aiidalab/app.py Outdated Show resolved Hide resolved
aiidalab/app.py Show resolved Hide resolved
tests/test_appclass.py Outdated Show resolved Hide resolved
tests/test_appdataclass.py Show resolved Hide resolved
Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Cheers!

@unkcpz unkcpz merged commit 3e29843 into main Mar 15, 2023
@unkcpz unkcpz deleted the fix/360/update-status-version-issue branch March 15, 2023 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update button doesn't respect version compatibility
3 participants