Add tests for MacroManager #5
Conversation
xeruf
force-pushed
the
macros-test
branch
4 times, most recently
from
913c223 to
ace6690
Compare
| @@ -110,6 +110,9 @@ class ControlObject : public QObject { | |||
| // Instantly sets the value of the ControlObject | |||
| static void set(const ConfigKey& key, const double& value); | |||
|
|
|||
| // Instantly sets the value of the ControlObject to 1 and then to 0 | |||
| static void toggle(const ConfigKey& key); | |||
There was a problem hiding this comment.
I don't think this is needed. And why is this in a test PR?
There was a problem hiding this comment.
It is here because I use it for the test.
And I think that it can be quite useful since many controls need to be set back to 0 before they can be invoked again.
There was a problem hiding this comment.
And I think that it can be quite useful since many controls need to be set back to 0 before they can be invoked again.
Which ones? I think most of them work even if they are already set to 1.0. Also, I'd argue that they should reset themselves to 0.0 instead of relying on external code to reset them.
@daschuer @uklotzde What do you think?
There was a problem hiding this comment.
The recording_toggle controls, same for hotcue_X_activate and many more (buttons). Setting it to 1 means pressed, back to 0 means released.
I don't think we should change anything about the behavior, it is fine as it is. You can't press a button while it is already pressed, so setting it back to 0 is vital to reproduce what is actually happening.
src/test/macromanager_test.cpp
Outdated
| EXPECT_EQ(mgr.m_macroRecordingState.load(), MacroState::Disabled); | ||
| mgr.notifyCueJump(handle, 0, 1); | ||
| EXPECT_EQ(mgr.m_activeChannel, nullptr); | ||
| EXPECT_EQ(mgr.getMacro().m_length, 0); | ||
| mgr.m_macroRecordingState.store(MacroState::Armed); |
There was a problem hiding this comment.
These tests are too tightly coupled with MacroManager internals and will be hard to maintain long term. I want tests to fail I made a mistake, not break compilation each time I change the name of a private member variable.
| EXPECT_EQ(mgr.m_macroRecordingState.load(), MacroState::Disabled); | |
| mgr.notifyCueJump(handle, 0, 1); | |
| EXPECT_EQ(mgr.m_activeChannel, nullptr); | |
| EXPECT_EQ(mgr.getMacro().m_length, 0); | |
| mgr.m_macroRecordingState.store(MacroState::Armed); | |
| EXPECT_EQ(mgr.getRecordingState(), MacroState::Disabled); | |
| mgr.notifyCueJump(handle, 0, 1); | |
| EXPECT_EQ(mgr.getActiveChannel(), nullptr); | |
| EXPECT_EQ(mgr.getMacro().getLength(), 0); | |
| mgr.setRecordingState(MacroState::Armed); |
There was a problem hiding this comment.
The problem is that I don't want to add superfluous methods just for the test. Some of the suggested things do make sense though. I'll see.
But if you rename the variable, don't you usually also rename the getters? And does your IDE not support proper refactoring, which automatically renames all occurences? ;)
Will be merged into mixxxdj#2873 once stable.