microsoft / microsoft/terminal
A few StateMachine unit tests don't fully test what they purport to
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 105k
- Forks
- 9.6k
- Avg merge
- 3d 17h
- Merged PRs (30d)
- 29
Description
I'm not sure if this is right place to report this, because it's not a bug in the app itself, but it's an issue I noticed in the unit tests, which I think may be considered a bug.
As a specific example, consider the TestCursorKeysMode test in OutputEngineTest.cpp:
TEST_METHOD(TestCursorKeysMode)
{
StatefulDispatch* pDispatch = new StatefulDispatch;
VERIFY_IS_NOT_NULL(pDispatch);
StateMachine mach(new OutputStateMachineEngine(pDispatch));
mach.ProcessString(L"\x1b[?1h", 5);
VERIFY_IS_TRUE(pDispatch->_fCursorKeysMode);
pDispatch->ClearState();
mach.ProcessString(L"\x1b[?1l", 5);
VERIFY_IS_FALSE(pDispatch->_fCursorKeysMode);
pDispatch->ClearState();
}
The second half of this test is assumedly meant to prove that the given escape sequence will reset the cursor keys mode. However, the _fCursorKeysMode flag is false by default, so you could replace that escape sequence with almost anything, including removing it altogether, and the test would still pass.
There are similar problems with TestCursorBlinking, TestCursorVisibility, and possibly also TestAltBufferSwapping.
When I added a similar test, I got around the problem by explicitly setting the flag I was testing to the opposite of what was expected, but I'm not sure if that is the best approach. Another possibility might be using std::optional<bool> instead of just a bool, so the default value could then be a nullopt, and it wouldn't be possible to pass the test based on the default value alone. The downside of that approach is the VERIFY steps become a little more complicated.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/terminal/parser/ut_parser/OutputEngineTest.cpp with TestCursorKeysMode, TestCursorBlinking, TestCursorVisibility, and TestAltBufferSwapping. Review how each state is initialized before the reset or disable sequence, then run the affected StateMachine unit tests. Done means each test fails when its sequence is removed or replaced, rather than passing from the default state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- testing
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100