Fixed/improved error parsing from API messages. #3922

adbridge · 2017-03-10T12:29:49Z

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 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 · 2017-03-10T12:30:44Z

@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?

theotherjimmy · 2017-03-10T19:32:12Z

tools/check_release.py

-        if response['result']['data']['task_complete']:
-
+
+        data = response['result']['data']


Agreed. Nested json stuff can get gross.

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

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...

@theotherjimmy I believe it's a guaranteed response field

theotherjimmy · 2017-03-10T19:34:48Z

tools/check_release.py

@@ -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.")


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

Yeah will do

theotherjimmy · 2017-03-10T19:36:29Z

tools/check_release.py

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

    return mbed, mbed_dev

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


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

@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
?

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

theotherjimmy · 2017-03-10T19:38:54Z

tools/check_release.py


                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")


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

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 !

theotherjimmy · 2017-03-10T19:40:07Z

tools/check_release.py


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


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

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

theotherjimmy · 2017-03-10T19:46:14Z

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

bridadan · 2017-03-10T19:57:13Z

tools/check_release.py

-    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")))


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

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.

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

lol. good evil laugh.

0xc0170 · 2017-03-14T09:02:57Z

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

…inor formatting updates.

adbridge · 2017-03-14T12:48:04Z

@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?

bridadan · 2017-03-14T15:03:03Z

tools/check_release.py

-    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:


Lovely 😄

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

adbridge · 2017-03-15T10:57:19Z

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

adbridge · 2017-03-15T15:36:04Z

@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)

compilation set.

adbridge · 2017-03-16T15:47:53Z

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

adbridge · 2017-03-16T17:08:17Z

@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- · 2017-03-22T11:34:19Z

@theotherjimmy Can you list changes needed here or approve?

theotherjimmy · 2017-03-22T19:02:20Z

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

Fixed/improved error parsing from API messages.

e3dbb82

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 requested review from theotherjimmy and bridadan March 10, 2017 12:29

0xc0170 added the needs: review label Mar 10, 2017

theotherjimmy approved these changes Mar 10, 2017

View reviewed changes

bridadan approved these changes Mar 10, 2017

View reviewed changes

Review comments: Add a child logger, close json file after reading, m…

ca6bfe0

…inor formatting updates.

adbridge requested review from theotherjimmy and bridadan March 14, 2017 12:47

bridadan reviewed Mar 14, 2017

View reviewed changes

Added running total for target being compiled.

9649d36

Add an ignore list so that sets of test, target can be excluded from the

828b7ac

compilation set.

bridadan approved these changes Mar 16, 2017

View reviewed changes

theotherjimmy approved these changes Mar 22, 2017

View reviewed changes

adbridge added ready for merge release-version: 5.4.2 and removed needs: review ready for merge labels Mar 23, 2017

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

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


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


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

		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:

Fixed/improved error parsing from API messages. #3922

Fixed/improved error parsing from API messages. #3922

Uh oh!

Conversation

adbridge commented Mar 10, 2017

Uh oh!

adbridge commented Mar 10, 2017

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Mar 10, 2017

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

bridadan Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 14, 2017

Uh oh!

adbridge commented Mar 14, 2017

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbridge commented Mar 15, 2017

Uh oh!

adbridge commented Mar 15, 2017

Uh oh!

adbridge commented Mar 16, 2017

Uh oh!

adbridge commented Mar 16, 2017

Uh oh!

sg- commented Mar 22, 2017

Uh oh!

theotherjimmy commented Mar 22, 2017

Uh oh!

Uh oh!

bridadan Mar 10, 2017 •

edited

Loading

theotherjimmy Mar 10, 2017 •

edited

Loading