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

test: add boundary check for implicit response method #548

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

tdb3
Copy link
Contributor

@tdb3 tdb3 commented Dec 3, 2024

Looks like #192 introduced changes to the stratum_method enum that causes failures in the unit tests added a week prior in #163. This PR adjusts adjusts the tests and makes them more descriptive to test the boundary condition in

} else if (!cJSON_IsNull(error_json)) {
if (parsed_id < 5) {
result = STRATUM_RESULT_SETUP;
} else {
result = STRATUM_RESULT;
}
message->response_success = false;
//if the result is a boolean, then parse it
} else if (cJSON_IsBool(result_json)) {
if (parsed_id < 5) {
result = STRATUM_RESULT_SETUP;
} else {
result = STRATUM_RESULT;
}

Tested by flashing unit_test_stratum.bin and monitoring test logging:

Running Parse stratum result success...
/home/dev/myrepos/ESP-Miner/components/stratum/test/test_stratum_json.c:118:Parse stratum result success:PASS
...
Running Parse stratum result error...
/home/dev/myrepos/ESP-Miner/components/stratum/test/test_stratum_json.c:155:Parse stratum result error:PASS

@eandersson
Copy link
Collaborator

Unrelated to the PR itself but could you add a guide on how to run the unit tests locally?

@tdb3
Copy link
Contributor Author

tdb3 commented Dec 3, 2024

Unrelated to the PR itself but could you add a guide on how to run the unit tests locally?

Added a unit test guide in #549

@eandersson
Copy link
Collaborator

Unrelated to the PR itself but could you add a guide on how to run the unit tests locally?

Added a unit test guide in #549

Perfect thanks. I'll try to check it out tomorrow.

@WantClue
Copy link
Collaborator

WantClue commented Jan 4, 2025

@tdb3 please resolve conflicts

Test the boundary of STRATUM_RESULT and STRATUM_RESULT_SETUP more completely
@tdb3 tdb3 force-pushed the 20241202_fix_tests_stratum_method branch from cdbc37a to 72f859d Compare January 4, 2025 05:05
@tdb3
Copy link
Contributor Author

tdb3 commented Jan 4, 2025

Thanks for pinging. Update pushed. Needed to rebase on top of commit f454b0c due to Issue #607. Merge conflict should be alleviated.

idf.py -p /dev/ttyACM0 monitor
...
Running Parse stratum result success...
/home/dev/myrepos/ESP-MinerAlt/ESP-Miner/components/stratum/test/test_stratum_json.c:118:Parse stratum result success:PASS
...
Running Parse stratum result error...
/home/dev/myrepos/ESP-MinerAlt/ESP-Miner/components/stratum/test/test_stratum_json.c:155:Parse stratum result error:PASS

Also fixes a bug in the test that used TEST_ASSERT_EQUAL instead of TEST_ASSERT_EQUAL_STRING for string checking.

@WantClue WantClue merged commit 3bf8997 into skot:master Jan 4, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants