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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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

Better test the boundary of STRATUM_RESULT and STRATUM_RESULT_SETUP
@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.

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