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

Fixed/improved error parsing from API messages. #3922

Merged
merged 4 commits into from Mar 24, 2017

Conversation

Projects
None yet
5 participants
@adbridge
Contributor

adbridge commented Mar 10, 2017

Fixed results output summary.
General tidy up of long lines.
Added a new field to json file, target_list, to allow the user to
override the set of targets used for the compilation.

Fixed/improved error parsing from API messages.
Fixed results output summary.
General tidy up of long lines.
Added a new field to json file, target_list, to allow the user to
override the set of targets used for the compilation.

@adbridge adbridge requested review from theotherjimmy and bridadan Mar 10, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 10, 2017

@theotherjimmy @bridadan please review. As well as the fixes added, incorporated the stylistic changes previously mentioned.

@theotherjimmy

Nice! I'm always down for linting. Any chance we can start unit testing this script?

if response['result']['data']['task_complete']:


data = response['result']['data']

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Agreed. Nested json stuff can get gross.

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Maybe a better name than data. but it matches the key...

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Are we garenteed to have response['result']['data'] here? it might be worth a try: except KeyError: or similar. Not blocking, but maybe we should consider it...

This comment has been minimized.

@adbridge

adbridge Mar 14, 2017

Contributor

@theotherjimmy I believe it's a guaranteed response field

@@ -270,7 +300,7 @@ def upgrade_test_repo(test, user, library, ref, repo_path):

os.rename(lib_file, bak_file)
else:
logging.error("!! Error trying to backup lib file prior to updating.")
logging.error("!! Failure to backup lib file prior to updating.")

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Maybe drop the !! here as well? It's kind of silly when we already do logging.error.

This comment has been minimized.

@adbridge

adbridge Mar 14, 2017

Contributor

Yeah will do

@@ -362,11 +394,22 @@ def get_latest_library_versions(repo_path):

return mbed, mbed_dev

def log_results(lst, title):
logging.info(title)

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

We should do the title and lst as one statement here. Otherwise it looks weird on the output.

This comment has been minimized.

@adbridge

adbridge Mar 14, 2017

Contributor

@theotherjimmy I don't understand your comment ? lst is checked for being empty (in which case output 'none', otherwise we need to iterate through the list printing entries . Are you suggesting printing title on every list entry line ?
E.g something more like:
FAILED: example_app
FAILED: other_example_app
SKIPPED: this_app
?

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 22, 2017

Contributor

Yes. I was thinking something like that. This would make it trivial to grep for failures/skipps/passes in a log file.


passes += (int)(result)
break
else:
logging.error("\t\tProgram/Target compilation failed due to internal errors. Removing from considered list!\n")
logging.error("\t\tCompilation failed due to internal errors.\n")
logging.error("\t\tSkipping test/target combination!\n")

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Can we drop the \t\t? If you want speciallized output styling there is always Config options.

This comment has been minimized.

@adbridge

adbridge Mar 14, 2017

Contributor

I don't think the config option do what I was intending. ie to make it stand out more to the human eye, but I don't really care that much to quibble about it !


logging.info(" SUMMARY OF COMPILATION RESULTS")
logging.info(" ------------------------------\n")

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Maybe drop the \n here? we already get a newline by calling logging.

This comment has been minimized.

@adbridge

adbridge Mar 14, 2017

Contributor

Same comment as previous, as to human eye readability . but again I don't really care that much..

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 10, 2017

Another requst: could we use something other than the root logger? LOG = logging.getlogger("check-release")?

@bridadan

LGTM, I agree with @theotherjimmy's feedback and noted one thing below. Not a blocker

json_data = json.load(open(os.path.join(os.path.dirname(__file__), "check_release.json")))

json_data = json.load(open(os.path.join(os.path.dirname(__file__),
"check_release.json")))

This comment has been minimized.

@bridadan

bridadan Mar 10, 2017

Contributor

We should probably close the file after opening it for reading.

json_file = open(os.path.join(os.path.dirname(__file__), "check_release.json"))
json_data = json.load(json_file)
json_file.close()

Or you can always do:

json_data  = None
with open(os.path.join(os.path.dirname(__file__), "check_release.json")) as json_file:
     json_data = json.load(json_file)

or something similar

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

Ahhhh!!! data?! you want to call it DATA!

It would actually be

with open(os.path.join(os.path.dirname(__file__), "check_release.json")) as config:
     json_data = json.load(config)

Also, open files that are leaked (not really gced language) are closed when gced. This is generally pretty safe, but the timing of closing the file descriptor can be wonky.

This comment has been minimized.

@bridadan

bridadan Mar 10, 2017

Contributor

I edited after you posted it so now you look silllly! Muhahaha

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 10, 2017

Contributor

lol. good evil laugh.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 14, 2017

whats the status of this patch? Once updated, please restart uvisor (if it does not retrigged by itself).

@adbridge adbridge requested review from theotherjimmy and bridadan Mar 14, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 14, 2017

@theotherjimmy @bridadan please re-review

@bridadan

Good changes! Though wouldn't the rel_log instance have to be declared at the top of the file? Or am I totally in the wrong on how Python scoping works?

json_data = json.load(open(os.path.join(os.path.dirname(__file__), "check_release.json")))


with open(os.path.join(os.path.dirname(__file__), "check_release.json")) as config:

This comment has been minimized.

@bridadan

bridadan Mar 14, 2017

Contributor

Lovely 😄

This comment has been minimized.

@adbridge

adbridge Mar 14, 2017

Contributor

It seems to work fine using the rel_log instance globally , so not sure !!

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 15, 2017

@marhil01 @SeppoTakalo Hi guys, looks like we are getting Oulu CI failures for unrelated issues to the PR under test ??

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 15, 2017

@theotherjimmy @bridadan Hold fire for a little while on the re-review as I have another change I want to bring in (to add support for an ignore list ie a list of test, target to ignore when running through the compilation set)

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 16, 2017

@theotherjimmy @bridadan ok good to go on the re-review thanks

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 16, 2017

@marhil01 @SeppoTakalo Looks like a Raas demon problem:

16:05:15 [wlan-8022]   File "/builds/ws/app_master-L2X4U6MCIG7IS377UB6AF/mbed-clitest/mbed_clitest/bench.py", line 750, in run
16:05:15 [wlan-8022]     self.__setUpBench()
16:05:15 [wlan-8022]   File "/builds/ws/app_master-L2X4U6MCIG7IS377UB6AF/mbed-clitest/mbed_clitest/bench.py", line 944, in __setUpBench
16:05:15 [wlan-8022]     self.__initDuts()
16:05:15 [wlan-8022]   File "/builds/ws/app_master-L2X4U6MCIG7IS377UB6AF/mbed-clitest/mbed_clitest/bench.py", line 556, in __initDuts
16:05:15 [wlan-8022]     self.duts = self.resource_provider.initialize_duts(self.DEFAULT_BIN)
16:05:15 [wlan-8022]   File "/builds/ws/app_master-L2X4U6MCIG7IS377UB6AF/mbed-clitest/mbed_clitest/ResourceProvider/ResourceProvider.py", line 95, in initialize_duts
16:05:15 [wlan-8022]     self.allocator = self.__get_allocator()
16:05:15 [wlan-8022]   File "/builds/ws/app_master-L2X4U6MCIG7IS377UB6AF/mbed-clitest/mbed_clitest/ResourceProvider/ResourceProvider.py", line 192, in __get_allocator
16:05:15 [wlan-8022]     return RaasAllocator(address = self.args.raas, pwd=pwd, user=user, token=token)
16:05:15 [wlan-8022]   File "/builds/ws/app_master-L2X4U6MCIG7IS377UB6AF/mbed-clitest/mbed_clitest/ResourceProvider/Allocators/RaasAllocator.py", line 31, in __init__
16:05:15 [wlan-8022]     raise AllocationError("Couldn't open connection to RaaS Daemon: {}".format(e))
16:05:15 [wlan-8022] AllocationError: Couldn't open connection to RaaS Daemon: 
16:05:15 
[wlan-8022] 16:05:14.800 | TC     MainThread: Test Case fails because of: Couldn't open connection to RaaS Daemon: 
16:05:15 [wlan-8022] 16:05:14.800 | TC     MainThread: ====tearDownTestBench====

Can you help please?

@sg-

This comment has been minimized.

Member

sg- commented Mar 22, 2017

@theotherjimmy Can you list changes needed here or approve?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 22, 2017

I thought I approved it. @sg- no changes requested at this time.

@adbridge adbridge merged commit a49144a into ARMmbed:master Mar 24, 2017

3 checks passed

Cam-CI uvisor Build & Test Success
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment