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

python scripts : table print with github policy #7720

Merged
merged 1 commit into from Sep 7, 2018

Conversation

@jeromecoutant
Contributor

jeromecoutant commented Aug 7, 2018

Description

This is following ARMmbed/greentea#277 discussion

Goal is to get test console print compatible with github like :

target platform_name test suite result elapsed_time (sec) copy_method
DISCO_L475VG_IOT01A-ARM DISCO_L475VG_IOT01A tests-mbed_drivers-lp_ticker OK 21.31 default
DISCO_L475VG_IOT01A-ARM DISCO_L475VG_IOT01A tests-mbed_drivers-lp_timeout OK 59.35 default

Pull request type

[ ] Fix
[x] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change
@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Aug 7, 2018

For information, here is all related PR :

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

Alright, this has simmered for long enough.

I like the change, but I'm not married to it one way or another.

@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test @theotherjimmy @bridadan @ARMmbed/device-os-custom-engineering-and-support @ARMmbed/mbed-os-core Any strong feelings one way or another?

Since this is a PR against all of the tools, I'd like it sorted out one way or another sooner rather than later.

@theotherjimmy

Changes look good. Copy-paste-able tables should have always been a design goal.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Aug 14, 2018

/morph build

@adbridge adbridge added needs: CI and removed needs: review labels Aug 14, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 14, 2018

Build : SUCCESS

Build number : 2805
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7720/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

/morph export-build

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

1 similar comment
@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 14, 2018

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Aug 15, 2018

Looks like this has failed due to some networking issues in CI. Suggest we re-run this once the CI is less heavily loaded.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 16, 2018

/morph test

@yennster

This comment has been minimized.

Contributor

yennster commented Aug 16, 2018

I like it, however the missing top/bottom lines on the compile build size information makes it a bit hard to read compared to the previous implementation (in my opinion):

New Old
screen shot 2018-08-16 at 9 45 25 am screen shot 2018-08-16 at 9 48 36 am

Not a big deal though!

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Aug 16, 2018

Maybe, we can add some empty line before and after array ?

@mbed-ci

This comment has been minimized.

@cmonr cmonr added the needs: work label Aug 17, 2018

@cmonr cmonr removed the needs: CI label Aug 17, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

https://github.com/ozh/ascii-tables So many ascii tables...

@jeromecoutant Could you add whitespace to make the CLI output a bit easier to discern?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 20, 2018

Maybe, we can add some empty line before and after array ?

👍 Please do

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

@jeromecoutant Let us know once updated.

For information, here is all related PR :

There's list of dependencies. @cmonr @theotherjimmy What should be the order (this goes first and then the rest or?) ?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 24, 2018

@0xc0170 Fortunately it's not a dependency list.

In this case, the Mbed OS tools and Mbed OS changes don't rely on one another. We only need to make sure that the tools and Mbed OS version that is released line up to present the "this is a new feature across our tools" story.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 24, 2018

A different question.

One or two of those PRs has/have now gone in, which is great, but that begs the question of whether we should still add the whitespace change request, or bring this in asis.

For the sake of time, I'd argue for bringing it in asis, since this would be ready for CI.

@cmonr cmonr added needs: review and removed needs: work labels Aug 24, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 24, 2018

@jamesbeyond

This comment has been minimized.

Contributor

jamesbeyond commented Aug 24, 2018

Just wonder why not just leave the top and bottom border in? it is more clear in the console output. and not doing any harm if you want to copy the table to markdown file. just don't copy those 2 lines.
image

@bridadan

This comment has been minimized.

Contributor

bridadan commented Aug 24, 2018

@jamesbeyond I think the idea is you can post the whole log and you get the table for free. If you had to delete lines then it wouldn't work out-of-the-box.

@cmonr Having a new line would be nice, but if there's a big push to get this in it can always be added in another PR. But if this isn't a high priority I don't have a problem with you waiting on it for a bit.

@yennster

This comment has been minimized.

Contributor

yennster commented Sep 5, 2018

Have we gotten those blank lines?

@cmonr cmonr added needs: CI and removed needs: review labels Sep 6, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 6, 2018

Nope, and now that the othre PRs have been merged, it looks like we should omit them to keep all of the tools homogeneous.

Will restart CI when able (we restart CI when past successful results in a PR are older than a week).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 6, 2018

Build : SUCCESS

Build number : 3022
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7720/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 6, 2018

Test failure not caused by PR. Requeueing.
/morph test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 6, 2018

/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 48c149b into ARMmbed:master Sep 7, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 595 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9787 cycles (-20 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Sep 7, 2018

@jeromecoutant jeromecoutant deleted the jeromecoutant:PR_PRETTYTABLE branch Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment