Skip to content

Testrunner refactor to support multiple targets #1212

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

Closed

Conversation

rtakacs
Copy link
Contributor

@rtakacs rtakacs commented Sep 23, 2017

This pull request is a follow-up patch for #1207. The main goal is to use the js-remote-test inside the iotjs project. This part of the mentioned approach is responsible for the testrunner modification to handle multiple devices for testing.

The architecture is the following:

  • tools/test_runner.py file - Defines only the options for the testing and starts the test execution.
  • tools/testdriver folder - Holds all the logics (files) that are needed to testing remotely.
  • tools/testdriver/testrunner.py file - This file is almost the same with the currently used testrunner.py, but the run_test method is removed. The reason is that this patch has more targets, and they should implement their own test execution (run_test method). Currently, just the host and the stm32f4dis targets are supported by this patch. The JS coverage measurement is also moved to the host target because only that target has enough memory.
  • tools/testdriver/target folder - Holds all the targets that should be supported by the testrunner. They should implement the run_test method.
  • tools/testdriver/target/connection folder - Holds the Python implementation of the ssh and serial communications.

Some examples for running the testrunner:

python test_runner.py --bin-path /path/to/iotjs                  # Testing on host
python test_runner.py --target stm32f4dis --port /dev/STM32F4    # Testing on stm board

This patch is in Work In Progress state yet. The reason of the early upload is to show the required modifications.

@rtakacs rtakacs force-pushed the remote_testrunner branch 4 times, most recently from aa9b02e to 5ae3819 Compare September 25, 2017 13:08
@rtakacs
Copy link
Contributor Author

rtakacs commented Sep 25, 2017

I've tested both the host and the stm32f4 target. They seem work. Just some outputs:

$ python tools/test_runner.py --bin-path build/x86_64-linux/debug/bin/iotjs --quiet

Test configuration:
  target:        host
  bin path:      build/x86_64-linux/debug/bin/iotjs
  coverage:      False
  valgrind:      False
  quiet:         True
  timeout:       300
  skip-modules:  ''

Testset: run_pass
  SKIP: test_adc.js   (Reason: need to setup test environment)
  PASS: test_assert.js (0.01s)
  SKIP: test_ble_advertisement.js   (Reason: need to setup test environment)
  SKIP: test_ble_setservices.js   (Reason: need to setup test environment)
  ...
$ python tools/test_runner.py --target stm32f4dis --port /dev/STM32F4 --quiet

Test configuration:
  target:        stm32f4dis
  port:          /dev/STM32F4
  baud:          115200
  quiet:         True
  timeout:       300
  skip-modules:  ''

Testset: run_pass
  SKIP: test_adc.js   (Reason: need to setup test environment)
  PASS: test_assert.js (0.09s)
  SKIP: test_ble_advertisement.js   (Reason: need to setup test environment)
  SKIP: test_ble_setservices.js   (Reason: need to setup test environment)
  SKIP: test_ble_setservices_central.js   (Reason: run it with nodejs after running test_ble_setservices.js)
  PASS: test_buffer_builtin.js (0.1s)
  ...

But first #1207 should be landed, to be able to build the targets. And, of course #1214, to skip file system tests because the test execution happens in the Read Only Memory.

@rtakacs
Copy link
Contributor Author

rtakacs commented Sep 26, 2017

#1219 is also needed to use the testrunner.

@hs0225
Copy link
Contributor

hs0225 commented Sep 27, 2017

@rtakacs js-remote-test support both iot.js and jerryscript. Will you remove js-remote-test repo and make this PR in jerryscript?

@rtakacs
Copy link
Contributor Author

rtakacs commented Sep 27, 2017

@hs0225 Yes, this is the original plan :) If iot.js and jerryscript could have a remote testrunner (like this patch), js-remote-test would be unnecessary, so we can remove that.

Please note that only the master branch should be removed later, gh-pages branch is neccessary for the webpage.

IoT.js-DCO-1.0-Signed-off-by: Roland Takacs [email protected]
@rtakacs rtakacs changed the title [WIP] Testrunner refactor to support multiple targets. Testrunner refactor to support multiple targets Sep 27, 2017
@rtakacs
Copy link
Contributor Author

rtakacs commented Sep 27, 2017

Okay, I've removed the the [WIP] prefix from the title. Just some notes:

By default, the testrunner doesn't create build for the selected target.
In case of the host target, the --bin-path helps to define the iotjs binary path.
In case of the stm32f4dis target, --port helps to define the serial port name (e.g. /dev/ttyACM0).

But, there is a --build option, that helps to build (and flash) the selected target.

Just some examples:

test_runner.py --build                   # Create a host build and use that for testing
test_runner.py --bin_path path/to/iotjs  # Test the given iotjs

test_runner.py --target stm32f4dis --port /dev/ttyACM0          # Test iotjs on stm32f4dis target
test_runner.py --target stm32f4dis --port /dev/ttyACM0 --build  # Create and flash a nuttx-iotjs build, and use that for testing

@glistening
Copy link
Contributor

@rtakacs I've read the PR and suggestion now since I leaved the office for 1 week. Till now, we've used js-remote-test both for jerry and iotjs. But this patch seems to try to make duplicated code in each repository to remove circular dependency. Is it right? We would like to keep js-remote-test as repository for common code. We would like to discuss about this change on IRC.

@rtakacs
Copy link
Contributor Author

rtakacs commented Sep 28, 2017

@glistening The plan is not to eliminate circular dependencies (that could be in js-remote-test). The plan is to remove the js-remote-test and moving all the necessary logics to iotjs and jerryscript. Now, of course, there are duplications between the iotjs and js-remote-test, because the js-remote-test is not removed yet.

The following points show what could be the duplications in iotjs and jerryscript:

  • Maybe the build-and-flash mechanism (downloading NuttX, ST-Link, building and flashing the device)
  • The jerry testrunner is different from the iotjs one. But the target folder (in this patch) should be in jerryscript as well, because that is responsible for the device specific output processing.

So yes, there could be some duplications between iotjs and jerryscript. If that is too much, we can add js-remote-test to iotjs and jerryscript as a submodule:

iotjs                                           jerryscript
  |---src                                           |---jerry-core
  |---tools                                         |---tools
       |--- js-remote-test                               |---js-remote-test

In this case js-remote-test needs to be refactored. We should remove the iotjs and jerryscript projects from js-remote-test (js-remote-test/projects/iotjs js-remote-test/projects/jerryscript) to eliminate circular dependencies. After that, the js-remote-test always should know that it is in a tools folder either in iotjs, or jerryscript.

A --project option could show to the js-remote-test which project (iotjs, jerryscript) contain that.

e.g.

$ iotjs/tools/js-remote-test/driver.py --project iotjs
$ jerryscript/tools/js-remote-test/driver.py --project jerryscript

@rtakacs
Copy link
Contributor Author

rtakacs commented Sep 28, 2017

I've just realized that there already is a --app option in the driver.py of js-remote-test, that can help to define the target application (iotjs, jerryscript):

e.g. python driver.py --app {iotjs, jerryscript} --device {stm32, rpi2, artik053}

So a new --project option is unnecessary.

@glistening
Copy link
Contributor

@rtakacs Thank you for nice explanation. I slightly prefer 2nd option since it removes duplication. Perhaps @yichoi also has his preference.

@rtakacs rtakacs closed this Oct 4, 2017
@rtakacs
Copy link
Contributor Author

rtakacs commented Oct 4, 2017

Closed this pull request based on the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants