intercept `val` in driver_interpreter and remove capitals #3654

levkropp · 2024-08-30T15:08:06Z

georgeliao · 2024-08-30T15:17:18Z

src/daemon/daemon_init_settings.cpp

@@ -53,6 +53,7 @@ QString persistent_settings_filename()

 QString driver_interpreter(QString val)
 {
+    val = val.toLower().remove("-");


This also accepts something like HyPeR-----V, right? Is that ok? I think we might need a regex format check on the input val first and then interpret.

Is that ok?

Since hyper-v is the only hypervisor that contains dashes IIRC, it might be better to only apply toLower() to val and then whenever there is a check for if (driver == "hyperv") to also check for if (driver == "hyperv" || driver == "hyper-v"). What do you think about this solution instead?

I would like to add a third option 🙂 How about we do only toLower() here and convert hyper-v to hyperv on the Windows platform?

That could even be done in a separate PR, to avoid having to sync branches.

Since hyper-v is the only hypervisor that contains dashes IIRC, it might be better to only apply toLower() to val and then whenever there is a check for if (driver == "hyperv") to also check for if (driver == "hyperv" || driver == "hyper-v"). What do you think about this solution instead?

This sounds good. One more hard-coded string is not that bad in this scenario.

convert hyper-v to hyperv on the Windows platform?

Do you mean removing the first - character from the input string via QString member function, and compare to hyperv string? If that can be easily done via QString functions, this is also good.

codecov · 2024-08-30T15:19:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.86%. Comparing base (a1df43d) to head (ddf2fc9).
Report is 19 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3654      +/-   ##
==========================================
- Coverage   88.89%   88.86%   -0.04%     
==========================================
  Files         254      254              
  Lines       14251    14266      +15     
==========================================
+ Hits        12668    12677       +9     
- Misses       1583     1589       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…river_interpeter

ricab

Thank you Lev, I haven't tested but this looks good to me. Fine with me if @georgeliao approves.

georgeliao · 2024-09-06T10:12:19Z

Thank you Lev, I haven't tested but this looks good to me. Fine with me if @georgeliao approves.

The only concern of mine is that we will accept all the partial upper case name cases like, hYPER-V, vBOx, virtUALBox, because we convert the whole word to lower cases. However, I think these names should be also acceptable.

src/daemon/daemon_init_settings.cpp

ricab · 2024-09-06T10:23:43Z

Thanks @georgeliao. I think we can indeed accept that, since no ambiguity should ever come from it.

@levkropp small detail inline if you want, then we can merge. Thanks!

ricab

Oh, tests missing. We missed Codecov before. This shouldn't be too hard: look for the tests that cover the surrounding code and it should be easy to replicate/generalize.

georgeliao

@levkropp
Thanks for the good work. I really like the patch 100% test coverage.

Functional testing was done on windows, it worked as expected.

Both the interpreter code and unit tests look good. I only had a few minor comments on unit tests for refinement.

georgeliao · 2024-09-11T10:01:34Z

tests/test_global_settings_handlers.cpp

@@ -267,6 +267,32 @@ TEST_F(TestGlobalSettingsHandlers, daemonRegistersHandlerThatAcceptsValidBackend
    ASSERT_NO_THROW(handler->set(key, val));
 }

+TEST_F(TestGlobalSettingsHandlers, daemonRegistersHandlerThatTransformsHyperVDriver)
+{
+    auto key = mp::driver_key, val = "hyper-v";


Two minor things here:

First one is the multiple variables declaration in one line. I know we have other code doing this and the code here is correct, but this is not a good practice in general in terms of readability and error proneness. There is also a c++ core guideline item for this.

Second one is that const can be added to the three to make these variables read only. That makes the result code:

const auto key = mp::driver_key; const auto val = "hyper-v"; const auto transformed_val = "hyperv";

georgeliao · 2024-09-11T10:05:38Z

tests/test_global_settings_handlers.cpp

+
+TEST_F(TestGlobalSettingsHandlers, daemonRegistersHandlerThatTransformsVBoxDriver)
+{
+    auto key = mp::driver_key, val = "vbox";


georgeliao · 2024-09-11T10:05:52Z

tests/test_global_settings_handlers.cpp

+
+    mp::daemon::register_global_settings_handlers();
+
+    EXPECT_CALL(*mock_qsettings, setValue(Eq(key), Eq(transformed_val)));


georgeliao · 2024-09-11T10:07:56Z

tests/test_global_settings_handlers.cpp

+
+    mp::daemon::register_global_settings_handlers();
+
+    EXPECT_CALL(*mock_qsettings, setValue(Eq(key), Eq(transformed_val)));


Another minor thing.

Without specifying the called times, the default expected called number is at least once. Maybe we can make it more accurate by specifying times once since it is only called once. That makes

EXPECT_CALL(*mock_qsettings, setValue(Eq(key), Eq(transformed_val))).Times(1)

levkropp linked an issue Aug 30, 2024 that may be closed by this pull request

Accept local.driver values regardless of the case #3565

Closed

georgeliao reviewed Aug 30, 2024

View reviewed changes

intercept val in driver_interpreter and remove capitals

6754877

levkropp force-pushed the local-driver-case-insensitivity branch from e6054db to 6754877 Compare August 30, 2024 15:45

levkropp changed the title ~~intercept val in driver_interpreter and remove capitals and dashes~~ intercept val in driver_interpreter and remove capitals Aug 30, 2024

change val from "hyper-v" to "hyperv" and "vbox" to "virtualbox" in d…

ed07c9e

…river_interpeter

ricab previously approved these changes Sep 5, 2024

View reviewed changes

georgeliao previously approved these changes Sep 6, 2024

View reviewed changes

ricab reviewed Sep 6, 2024

View reviewed changes

src/daemon/daemon_init_settings.cpp Outdated Show resolved Hide resolved

levkropp dismissed stale reviews from georgeliao and ricab via 4696cf9 September 6, 2024 10:44

ricab reviewed Sep 6, 2024

View reviewed changes

levkropp force-pushed the local-driver-case-insensitivity branch from 4696cf9 to 5b25bd9 Compare September 10, 2024 18:27

georgeliao requested changes Sep 11, 2024

View reviewed changes

add tests for "hyper-v" and "vbox" cases

ddf2fc9

levkropp force-pushed the local-driver-case-insensitivity branch from 5b25bd9 to ddf2fc9 Compare September 11, 2024 12:04

georgeliao approved these changes Sep 11, 2024

View reviewed changes

georgeliao added this pull request to the merge queue Sep 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 11, 2024

georgeliao added this pull request to the merge queue Sep 11, 2024

Merged via the queue into main with commit 04b3227 Sep 11, 2024
13 of 14 checks passed

georgeliao deleted the local-driver-case-insensitivity branch September 11, 2024 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intercept `val` in driver_interpreter and remove capitals #3654

intercept `val` in driver_interpreter and remove capitals #3654

levkropp commented Aug 30, 2024

georgeliao Aug 30, 2024

levkropp Aug 30, 2024

ricab Aug 30, 2024

ricab Aug 30, 2024

georgeliao Aug 30, 2024

georgeliao Aug 30, 2024

codecov bot commented Aug 30, 2024 •

edited

Loading

ricab left a comment

georgeliao commented Sep 6, 2024

ricab commented Sep 6, 2024

ricab left a comment

georgeliao left a comment •

edited

Loading

georgeliao Sep 11, 2024

georgeliao Sep 11, 2024

georgeliao Sep 11, 2024

georgeliao Sep 11, 2024


		mp::daemon::register_global_settings_handlers();

		EXPECT_CALL(*mock_qsettings, setValue(Eq(key), Eq(transformed_val)));

intercept val in driver_interpreter and remove capitals #3654

intercept val in driver_interpreter and remove capitals #3654

Conversation

levkropp commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

ricab left a comment

Choose a reason for hiding this comment

georgeliao commented Sep 6, 2024

ricab commented Sep 6, 2024

ricab left a comment

Choose a reason for hiding this comment

georgeliao left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intercept `val` in driver_interpreter and remove capitals #3654

intercept `val` in driver_interpreter and remove capitals #3654

codecov bot commented Aug 30, 2024 •

edited

Loading

georgeliao left a comment •

edited

Loading