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

Add target and pass_string parameters to the renode test script to fix #46186 #46187

Merged

Conversation

advaitjain
Copy link
Member

@advaitjain advaitjain commented Jan 5, 2021

This allows the test_with_renode.sh script to be called via the makefile with all the parameters needed to run on a given target, while also staying consistent with the other test scripts.

As a result of this change, the makefile is passing in the following parameters to the test scripts:
param 1 - test binary path
param 2 - string to determine that test passes
param 3 - target

Parameter 3 is only used for test_with_renode.sh

Manually tested that the following commands now pass:

make -f tensorflow/lite/micro/tools/make/Makefile TARGET=bluepill test_kernel_add_test
make -f tensorflow/lite/micro/tools/make/Makefile TARGET=stm32f4 TAGS=cmsis-nn test_kernel_fully_connected_test

Fixes #46186

…tensorflow#46186

This allows the `test_with_renode.sh` script to be called via the
makefile with all the parameters needed to run on a given target, while
also staying consistent with the other test scripts.

As a result of this change, the makefile is passing in the following
parameters to the test scripts:
 #1 - test binary path
 tensorflow#2 - string to determine that test passes
 tensorflow#3 - target

Parameter tensorflow#3 is only used for `test_with_renode.sh`

Manually tested that the following commands now pass:
```
make -f tensorflow/lite/micro/tools/make/Makefile TARGET=bluepill test_kernel_add_test
make -f tensorflow/lite/micro/tools/make/Makefile TARGET=stm32f4 TAGS=cmsis-nn test_kernel_fully_connected_test
```
@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Jan 5, 2021
@google-ml-butler
Copy link

Thanks for contributing to TensorFlow Lite Micro.

To keep this process moving along, we'd like to make sure that you have completed the items on this list:

We would like to have a discussion on the Github issue first to determine the best path forward, and then proceed to the PR review.

@google-cla google-cla bot added the cla: yes label Jan 5, 2021
@advaitjain advaitjain added the comp:micro Related to TensorFlow Lite Microcontrollers label Jan 5, 2021
@advaitjain advaitjain linked an issue Jan 5, 2021 that may be closed by this pull request
Copy link
Contributor

@njeffrie njeffrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Curious about my one comment, possibly I'm missing something about the existing implementation.

@@ -479,7 +479,7 @@ $(1): $$($(1)_BINARY)
$(1)_bin: $$($(1)_BINARY).bin
test_$(1): $$($(1)_BINARY)
@test -f $$(TEST_SCRIPT) || (echo 'Unable to find the test script. Is the software emulation available in $$(TARGET)?'; exit 1)
$$(TEST_SCRIPT) $$($(1)_BINARY) '~~~ALL TESTS PASSED~~~'
$$(TEST_SCRIPT) $$($(1)_BINARY) $$(TEST_PASS_STRING) $$(TARGET)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're weren't passing TARGET in before. I'm confused how this worked previously.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked for everything but running tests with renode (which is the only place where we need TARGET.

Even with renode, it works when we call make test which is what is part of our CI since that uses a different test rule which does pass in the target:

TARGET_SPECIFIC_MAKE_TEST := 1
test: build
$(TEST_SCRIPT) $(BINDIR) $(TARGET)

This common code in helper_functions.inc is only used when we do:

make -f tensorflow/lite/micro/tools/make/Makefile TARGET=bluepill test_kernel_add_test

which is what this change is fixing.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jan 5, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 5, 2021
@gbaned gbaned self-assigned this Jan 6, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jan 6, 2021
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jan 6, 2021
@copybara-service copybara-service bot merged commit 54eefc7 into tensorflow:master Jan 6, 2021
PR Queue automation moved this from Assigned Reviewer to Merged Jan 6, 2021
@advaitjain advaitjain deleted the fix-renode-single-test branch January 29, 2021 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:micro Related to TensorFlow Lite Microcontrollers ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

running a single test with renode is broken.
4 participants