-
Notifications
You must be signed in to change notification settings - Fork 171
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
Icetea added #659
Icetea added #659
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not yet finished editing these files, but I've left some comments for you to resolve. Please address these across all files.
docs/tools/testing/testing_icetea.md
Outdated
|
||
You can run automated tests through Mbed CLI with the `--icetea` option. You can find more information about this in the [Mbed CLI test documentation] (/docs/tools/offline/cli-test-debug.md). | ||
|
||
The testing process requires tests to be built and that a test specification JSON file exist that describes these available tests. See the [test specification format](https://github.com/ARMmbed/greentea#test-specification-json-formatted-input). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We try not to link to GitHub. Is there another link we can use, instead? Maybe Doxygen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jupe do you know another place to link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some specific greentea documentation? No idea
docs/tools/testing/testing_icetea.md
Outdated
|
||
The test specification JSON is similar for the Greentea and Icetea tests. | ||
|
||
The Icetea tool handles the actual testing process. To read more about this tool, please visit its [GitHub repository](https://github.com/ARMmbed/icetea). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We try not to link to GitHub. Is there another link we can use, instead? Maybe Doxygen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment we doesn't have any other place where icetea documentation are available. We can generate html documentation using Sphinx - all configurations are already in place - just need some place which could share them. Suggestions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs site can build Sphinx
docs/tools/testing/testing_icetea.md
Outdated
|
||
##### Writing test cases | ||
|
||
For writing your own Python test scripts, you can read further information from [Icetea Test Case API](https://github.com/ARMmbed/icetea/blob/master/doc/tc_api.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We try not to link to GitHub. Is there another link we can use, instead? Maybe Doxygen?
docs/tools/testing/testing_icetea.md
Outdated
|
||
##### Writing CLI applications | ||
|
||
To be able to run the commands given to the DUT, you need to provide a CLI application that runs on the DUT. There is a library that you can use to create your own CLI application; it is called [mbed-client-cli](https://github.com/ARMmbed/mbed-client-cli). You can also find example Mbed CLI applications from the `mbed-os` repository TODO: Update before publishing [test applications](https://github.com/ARMmbed/mbed-os-icetea-integration/tree/feature-icetea/TEST_APPS/device). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We try not to link to GitHub. Is there another link we can use, instead? Maybe Doxygen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please resolve this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in this case github link is what we need. @jarlamsa do you know any other place to link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I do not. @jupe do you store generated docs of icetea somewhere else than github?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not at the moment. Maybe we should eventually.
docs/tools/testing/icetea.md
Outdated
|
||
### Documentation | ||
|
||
You can build HTML documentation for Icetea using sphinx. The source for the documentation is located in [doc-source](https://github.com/ARMmbed/icetea/tree/master/doc-source). For installation of sphinx, please see the [installation instructions](#installation). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? None of our other tools build documentation like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't understand why they need to build it. Why don't we publish it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is copied from icetea repository. There is separated issue about reviewing it IOTDOCS-459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address my comments before I continue reviewing.
docs/tools/testing/icetea.md
Outdated
|
||
To debug DUT locally with [GDB](https://www.gnu.org/software/gdb/): | ||
|
||
**Note:** You have to install [GDB](https://www.gnu.org/software/gdb/) first (`apt-get install gdb`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this must happen first, we should mention it first.
docs/tools/testing/icetea.md
Outdated
|
||
To analyze memory leaks with valgrind: | ||
|
||
**Note:** You have to install [valgrind](http://valgrind.org) first (`apt-get install valgrind`): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this must happen first, we should mention it first.
docs/tools/testing/icetea.md
Outdated
|
||
#### Dependencies | ||
|
||
Unit tests depend on mock, coverage and netifaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these? This is the only time we mention them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python modules required by unit tests of Icetea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonikula it is better to move this kind of information in some contribution guideline? I think that icetea unit testing is irrelevant for people who are just using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps. Something to discuss at least, for phase 2 of Icetea development. Not really a pressing issue now, I guess.
docs/tools/testing/icetea.md
Outdated
|
||
### License | ||
|
||
See the [license](https://github.com/ARMmbed/icetea/blob/master/LICENSE) agreement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have a different license than the rest of our tools? Can we add its information to the contributing section with the rest of our licensing content?
docs/tools/testing/icetea.md
Outdated
|
||
### Examples | ||
|
||
**Note:** The following examples use the [`dummyDut`](https://github.com/ARMmbed/icetea/blob/master/test/dut/dummyDut.c) application. It works in unix systems. To run the following examples, please compile `dummyDut` using Make: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a proper example in teams instead of linking to GH?
docs/tools/testing/icetea.md
Outdated
|
||
|
||
``` | ||
dut[number] #dut index (1..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add more context to this code snippet? I'm not sure what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're args for the execute_command/command function.
docs/tools/testing/icetea.md
Outdated
|
||
`icetea --help` | ||
|
||
All CLI parameters are described in [icetea.md](https://github.com/ARMmbed/icetea/blob/master/doc/Icetea.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't like to link to GH. Is there somewhere in the documentation we can link, instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few change requests, but otherwise looks good!
1. Verify the functionality of your implementation using the [Nanostack RF driver test application](https://github.com/ARMmbed/nanostack-rf-driver-tester). (This is currently only available to Mbed OS Partners.) | ||
1. Verify the functionality of your implementation by running the Nanostack RF driver testcase set in the Mbed OS repository: | ||
|
||
`mbed test --clean --compile --run --icetea -t <toolchain> -m <platform> --test-config MAC_TESTER -n address_read_and_write,send_data,send_data_indirect,send_large_payloads,create_and_join_PAN,ED_scan` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it'd be simpler to omit --compile --run
since that is the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
docs/tools/offline/cli-test-debug.md
Outdated
@@ -2,6 +2,10 @@ | |||
|
|||
Use the `mbed test` command to compile and run tests. | |||
|
|||
There are two testing frameworks, Greentea and Icetea. Greentea offers tests designed for driver porting and target verification. | |||
Icetea offers and manages tests that can contain multiple devices under test (DUTs) at the same time. In example you can test a network setup with a server and multiple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a funky new line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
docs/tools/offline/cli-test-debug.md
Outdated
@@ -99,6 +145,12 @@ You can specify to only **build** the tests by using the `--compile` option: | |||
$ mbed test -m K64F -t GCC_ARM --compile | |||
``` | |||
|
|||
In case of icetea the tests itself is not build but the tests requiring application is build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the phrasing to be a bit simpler here: "In case of icetea, only the test applications are built:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
docs/tools/testing/icetea.md
Outdated
|
||
# TODO: | ||
# 1. Finalize https://github.com/ARMmbed/icetea/blob/master/README.md | ||
# 2. Copy https://github.com/ARMmbed/icetea/blob/master/README.md here when ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be here?
docs/tools/testing/testing_icetea.md
Outdated
|
||
##### Writing CLI applications | ||
|
||
To be able to run the commands given to the DUT, you need to provide a CLI application that runs on the DUT. There is a library that you can use to create your own CLI application; it is called [mbed-client-cli](https://github.com/ARMmbed/mbed-client-cli). You can also find example Mbed CLI applications from the `mbed-os` repository [test applications](https://github.com/ARMmbed/mbed-os/tree/master/TEST_APPS/device). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mbed-client-cli
will now be present in Mbed OS. Perhaps we should link to that folder instead of the old repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository contains documentation for it. It is not included in the mbed-os.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, are there any plans to add that documentation to Mbed OS? Seems like it'd still be useful there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually know. @jupe do you know about this better?
docs/tools/testing/testing_icetea.md
Outdated
mbed export -i <IDE name> | ||
``` | ||
|
||
You can find your exported project in `projectfiles/<IDE>_<target>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct anymore. It should be removed from the greentea document as well, though that can be a separate change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "You can find your exported project files in the same folder you ran the export command" or totally removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "You can find your exported project in the root project directory." to match the patches I just pushed: #688
docs/tools/offline/cli-test-debug.md
Outdated
@@ -2,6 +2,8 @@ | |||
|
|||
Use the `mbed test` command to compile and run tests. | |||
|
|||
There are two testing frameworks: Greentea and Icetea. Greentea provides tests designed for driver porting and target verification. Icetea provides and manages tests for multiple devices under the test (DUTs) at the same time. For example, you can test the network setup for a server and multiple clients, and simultaneously control them from the test environment. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by 'devices under the test (DUTs)'? Is this just what devices that are currently being tested are called? The sentence is repetitive, so I would remove this or make a new sentence explaining this. So: "Icetea provides and manages tests for multiple devices at the same time. Devices being tested are referred to as devices under test (DUTs)."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DUT is acronym for device under test. I don't see the sentence being repetitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're saying it's testing devices under test - that's repetitive. That's why I think it would help to break it into two sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but the second sentence is similarly repetitive now, I don't see the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point it to spread out how many times you're repeating 'testing' in the same sentence. If you want to explain the acronym DUTs it would be better in a separate sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jen3andruska. Also, the DUT acronym isn't used later in this document, so how about we just leave that out? So just shorten it to "Icetea provides and manages tests for multiple devices at the same time."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it is defined mostly because the icetea tool itself uses the acronym.
docs/tools/offline/cli-test-debug.md
Outdated
The arguments to `test` are: | ||
* `-m <MCU>` to select a target for the compilation. If `detect` or `auto` parameter is passed, then Mbed CLI will attempt to detect the connected target and compile against it. | ||
* `-t <TOOLCHAIN>` to select a toolchain (of those defined in `mbed_settings.py`, see above), where `toolchain` can be either `ARM` (Arm Compiler 5), `GCC_ARM` (GNU Arm Embedded), or `IAR` (IAR Embedded Workbench for Arm). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above where? I don't see anything above this talking about mbed_settings.py
docs/tools/offline/cli-test-debug.md
Outdated
@@ -79,6 +86,32 @@ Test Case: | |||
Path: .\TESTS\functional\test3 | |||
``` | |||
|
|||
And in case of icetea: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "in case of icetea"? Do you mean to 'to find the tests that are available' for it, as in the previous example?
I made a comment on the same section in my review, Jen, with a suggested change--would love to get your input once I actually submit it :) |
@MelindaWeed Sounds good, I'll take a look when you submit it ;) |
docs/tools/offline/cli-test-debug.md
Outdated
@@ -91,6 +124,17 @@ mbedgt: available tests for built 'K64F-ARM', location '.\build\tests\K64F\ARM' | |||
test 'TESTS-functional-test3' | |||
``` | |||
|
|||
In case of icetea: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same ? as above: what do you mean by "in case of icetea"? Do you mean to 'to find the tests that are available' for it, as in the previous example?
docs/tools/offline/cli-test-debug.md
Outdated
@@ -79,6 +86,32 @@ Test Case: | |||
Path: .\TESTS\functional\test3 | |||
``` | |||
|
|||
And in case of icetea: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"In case of" usually means "if _____ happens".
"In the case of" (without emphasis) would be more correct here. However, I'd just go with "For icetea:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed,
docs/tools/offline/cli-test-debug.md
Outdated
@@ -99,6 +143,12 @@ You can specify to only **build** the tests by using the `--compile` option: | |||
$ mbed test -m K64F -t GCC_ARM --compile | |||
``` | |||
|
|||
In case of icetea only the test applications are built |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please phrase sentences preceding code snippets so you can end them with :
For icetea, only the test applications are built:
your code here
docs/tools/offline/cli-test-debug.md
Outdated
test 'TCPSERVER_ACCEPT' | ||
test 'TCPSOCKET_ECHOTEST_BURST_SHORT' | ||
``` | ||
|
||
### Compiling and running tests | ||
|
||
You can specify to only **build** the tests by using the `--compile` option: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"by using" -> with
This is nitpicky, admittedly. "By using" isn't wrong, just a little wordy.
docs/tools/testing/icetea.md
Outdated
@@ -0,0 +1,188 @@ | |||
## Icetea test framework | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What sort of line endings do you have here? They look like they're displaying a bit oddly.
docs/tools/testing/icetea.md
Outdated
automation in a Continuous Integration Environment. | ||
|
||
When testing [`Mbed OS`](https://www.mbed.com/en/platform/mbed-os/) | ||
Icetea allows you to execute commands remotely via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use via; go for non-Latin terms like through, with, or using.
docs/tools/testing/icetea.md
Outdated
|
||
**Using metadata filters** | ||
|
||
To run all test cases with testtype regression in the metadata: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put testtype
in bunny ears since it's a parameter (or whatever the correct word is), so it doesn't look like a typo.
docs/tools/testing/icetea.md
Outdated
|
||
#### Optional | ||
|
||
* If you wish to decorate your console log with all kinds of colors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"All kinds of" is a bit too informal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already changed this once, apparently some commit since reverted that.
docs/tools/testing/icetea.md
Outdated
#### Optional | ||
|
||
* If you wish to decorate your console log with all kinds of colors, | ||
install the coloredlogs module using pip. `pip install coloredlogs` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend putting module names in `` but I'm not sure what our protocol is for that, or if we have one.
docs/tools/testing/icetea.md
Outdated
* If you wish to decorate your console log with all kinds of colors, | ||
install the coloredlogs module using pip. `pip install coloredlogs` | ||
* There have been issues with coloredlogs installation on Windows. | ||
We might switch to a different module at some point to enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't make promises, especially if there's no set time for the solution.
docs/tools/testing/icetea.md
Outdated
|
||
`icetea --list --tcdir examples` | ||
|
||
To print Icetea version: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icetea has been lowercase in other places and is uppercase in this doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, it's capitalized if it's the first word of the sentence, but how is it officially styled otherwise?
I've put some of my comments through but am not done quite yet. |
Might not be able to finish before I have to leave for the day. |
docs/tools/offline/cli-test-debug.md
Outdated
* `-vv` or `--very_verbose` for very verbose diagnostic output. | ||
|
||
Invoke `mbed test`: | ||
* `-m <MCU>`: to select a target for the compilation. If the `detect` or `auto` parameter is passed, then Mbed CLI will attempt to detect the connected target and compile against it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to review these in Atom's Markdown preview to make sure these render as a list and not on a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Atom is a different flavour of MD from our engine. As best practice we always add an empty line.
docs/tools/testing/icetea.md
Outdated
It automates the process of flashing Mbed boards, running tests | ||
and accumulating test results into reports. | ||
Developers use it for local development as well as for | ||
automation in a Continuous Integration Environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well as automation -- you can leave out "for"
docs/tools/testing/icetea.md
Outdated
|
||
When testing [`Mbed OS`](https://www.mbed.com/en/platform/mbed-os/) | ||
Icetea allows you to execute commands remotely via | ||
the command line interface (`CLI`) in a device under test (`DUT`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these terms need to be in bunny ears ``
docs/tools/testing/icetea.md
Outdated
* Linux | ||
* python-dev and python-lxml | ||
`sudo apt-get install python-dev python-lxml` | ||
* In order to run test cases with hardware in Linux without sudo rights: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sudo rights is jargony.
docs/tools/testing/icetea.md
Outdated
`STATIC_DEPS=true sudo pip install lxml` | ||
|
||
* Windows | ||
* python-lxml installation is problematic on Windows since |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize Python unless it's the name of a module, in which case I'd put it in ``
docs/tools/testing/icetea.md
Outdated
install the coloredlogs module using pip. `pip install coloredlogs` | ||
* There have been issues with coloredlogs installation on Windows. | ||
We might switch to a different module at some point to enable | ||
colored logging on Windows as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color-coded
docs/tools/testing/icetea.md
Outdated
|
||
### Installation | ||
|
||
`> pip install icetea` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you pointed out already that you need pip
installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already changed this once, apparently some commit since then reverted that.
docs/tools/offline/cli-test-debug.md
Outdated
@@ -99,6 +143,12 @@ You can specify to only **build** the tests by using the `--compile` option: | |||
$ mbed test -m K64F -t GCC_ARM --compile | |||
``` | |||
|
|||
In case of icetea only the test applications are built |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, what do you mean by "In case of Icetea..."? If you mean regarding or concerning Icetea, then you need 'the':" in [the] case of... ", but this is still not the best way to say this. "In case" is only use without 'the' when saying something like "In case you were wondering..." (it's followed by a noun/subject).
So, are you saying that: 1) if you use the same compile option as above, only the test applications will build in Icetea? Or, 2) If they want to specify that only the test applications build in Icetea, then they should use the following command? (Is this elaborating on the previous command, or is it its own separate instruction).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 distinct tools, greentea and icetea. Greentea is used by default and to run some commands for icetea you need to use some command line arguments for that.
docs/tools/testing/icetea.md
Outdated
|
||
### Usage | ||
|
||
To print the help page: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say display instead of print, even though "print" is the codeish for that functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already changed this once, apparently some commit since then reverted that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it again for you. Hopefully that commit will stick.
docs/tools/testing/icetea.md
Outdated
|
||
**Running an example test case with hardware** | ||
|
||
In this example, we assume that a compatible board has been connected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passive -> active, please
"We assume you have connected a compatible board..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already changed this once, apparently some commit since then reverted that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird. I changed it again, so hopefully that commit will stick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, are you both editing directly on github or do you have a local copy on your PC?
I'm happy to go through this again when all of the updates are completed. Would you mind mentioning me in a comment if you'd like me to review this again? |
docs/tools/testing/icetea.md
Outdated
|
||
#### Typical use | ||
|
||
All of the commands described below might also need other options, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Options" is the wrong word here and sounds awkward (commands don't have options). When you say "other options," do you mean other adjustments, commands, compilations...? I think we need a better word here.
docs/tools/testing/icetea.md
Outdated
|
||
**Running a pre-made suite** | ||
|
||
Icetea supports a suite-file that describes a number of test cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "suite-file that describes a suite..." is very repetitive, so I changed the second 'suite' to 'number.'
docs/tools/testing/icetea.md
Outdated
|
||
**Further details** | ||
|
||
For further details on any of these features, see our documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should include a link to our documentation, so: "...see our [documentation](link to our documentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There used to be a link here. I was told to remove it since the link is already at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who said to remove it? Whenever we say "see our documentation," we always link. Unless...is this the documentation (this page)? Or was it linking to another page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peknis01 . I can add the links if needed.
Copy edit file, mostly for formatting.
Copy edit file, mostly for consistent branding and formatting.
Make remaining requested changes from comments.
Make requested changes, and make a few other copy edits.
e535e38
to
2bc593c
Compare
Rebased and I made quick look and looks good. Did not have time to read fully now |
@OPpuolitaival Thanks. I'll merge for OOB. If you see anything that needs tweaking, please let me know or put up a PR. |
No description provided.