Skip to content

Use JournalStorage in test_cli.py#5990

Merged
not522 merged 6 commits intooptuna:masterfrom
sawa3030:change/storage-type-in-test
Feb 25, 2025
Merged

Use JournalStorage in test_cli.py#5990
not522 merged 6 commits intooptuna:masterfrom
sawa3030:change/storage-type-in-test

Conversation

@sawa3030
Copy link
Copy Markdown
Collaborator

@sawa3030 sawa3030 commented Feb 20, 2025 •
edited
Loading

Motivation

  • Improve CI performance by replacing RDBStorage with JournalStorage in test_cli.py.

Description of the changes

  • Most instances of RDBStorage in test_cli.py have been replaced with JournalStorage to enhance efficiency.
  • Exceptions:
    • Tests that do not explicitly define a storage type, such as test_create_study_command_without_storage_url().
    • Tests that require a specific storage type, such as test_storage_upgrade_command().
  • Remove the assertion of study_id because the initial study_id is not defined in the code, making the test fragile.

@sawa3030 sawa3030 force-pushed the change/storage-type-in-test branch from f45058c to 6bd4828 Compare February 20, 2025 08:26
@sawa3030
Copy link
Copy Markdown
Collaborator Author

sawa3030 commented Feb 20, 2025

By comparing the previous CI run, which is done here, we could see that this PR contributes to improving CI speed.

Before (RDBStorage) After (JournalStorage)
Mac tests 9m48s 9m17s
Windows tests 30m41s 22m26s
Tests with minimum versions 14m 13m12s

@sawa3030 sawa3030 marked this pull request as ready for review February 20, 2025 09:39
@not522 not522 self-assigned this Feb 21, 2025
not522
not522 reviewed Feb 21, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!
Could you adding updates for test_tell and test_tell_with_nan in the same way?

@not522 not522 changed the title Use JournalStorage in test_cli.py Use JournalStorage in test_cli.py Feb 21, 2025
@not522 not522 added the test Unit test. label Feb 21, 2025
@c-bata
Copy link
Copy Markdown
Member

c-bata commented Feb 21, 2025 •
edited
Loading

@.kAIto47802 Could you review this PR? Sorry, I forgot you are busy at the moment.

@c-bata
Copy link
Copy Markdown
Member

c-bata commented Feb 21, 2025

@fusawayugo Could you review this PR?

@sawa3030
Copy link
Copy Markdown
Collaborator Author

sawa3030 commented Feb 21, 2025

@not522 Thank you! I've just made the update.

fusawa-yugo
fusawa-yugo approved these changes Feb 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@fusawa-yugo fusawa-yugo left a comment

Choose a reason for hiding this comment

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

LGTM!

not522
not522 reviewed Feb 21, 2025
View reviewed changes
tests/test_cli.py Outdated
Comment on lines +1147 to +1148
with StorageSupplier("journal") as storage:
assert isinstance(storage, JournalStorage)
Copy link
Copy Markdown
Member

@not522 not522 Feb 21, 2025

Choose a reason for hiding this comment

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

This storage is not used, so how about just removing it?

not522
not522 approved these changes Feb 25, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

LGTM!

@not522 not522 merged commit d763594 into optuna:master Feb 25, 2025
14 checks passed
@not522 not522 added this to the v4.3.0 milestone Mar 7, 2025
@sawa3030 sawa3030 deleted the change/storage-type-in-test branch November 14, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@not522 not522 not522 approved these changes

+1 more reviewer

@fusawa-yugo fusawa-yugo fusawa-yugo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@not522 not522

Labels

test Unit test.

Projects

None yet

Milestone

v4.3.0

Development

Successfully merging this pull request may close these issues.

5 participants

@sawa3030 @c-bata @not522 @fusawa-yugo @HideakiImamura