Merge lp:~oem-qa/checkbox/patch_exit_on_shorthand_file_not_found into lp:checkbox
- patch_exit_on_shorthand_file_not_found
- Merge into trunk
Status: | Rejected |
---|---|
Rejected by: | Javier Collado |
Proposed branch: | lp:~oem-qa/checkbox/patch_exit_on_shorthand_file_not_found |
Merge into: | lp:checkbox |
Diff against target: |
38 lines (+14/-0) 1 file modified
checkbox/application.py (+14/-0) |
To merge this branch: | bzr merge lp:~oem-qa/checkbox/patch_exit_on_shorthand_file_not_found |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Javier Collado (community) | Disapprove | ||
Daniel Manrique (community) | Needs Information | ||
Review via email: mp+41928@code.launchpad.net |
Commit message
Description of the change
When a user mistypes the name of the whitelist/blacklist file in the command line:
- checkbox execution isn't stopped even if the file doesn't exist
- just an info line is displayed in logs: "Failed to open file '%s': %s"
- whitelist/blacklist patterns aren't applied (without providing any feedback regarding the reason to the user)
With this change:
- filenames passed through the command line are checked
- if a file doesn't exist an error message is printed to stderr and the application exits
- 860. By Marc Tardif
-
Changed connection request back to use the full url instead rather than the path.
- 861. By Marc Tardif
-
Replaced external plugin with remote plugin in autotest and ltp scripts.
- 862. By Marc Tardif
-
Merged from testsprint-
checkbox- base-sru- changes.
Javier Collado (javier.collado) wrote : | # |
- 863. By Marc Tardif
-
Updated parsing of config parameters which fixes bug #689140.
- 864. By Marc Tardif
-
Merged from trunk.
- 865. By Marc Tardif
-
Merged from whitelist_
file_comments_ support. - 866. By Marc Tardif
-
Fixed jobs_info plugin to strip commented lines in whitelist and blacklist files which might start with a space.
- 867. By Marc Tardif
-
Added additional logging to the reactor when firing messages.
- 868. By Marc Tardif
-
Merged from checkbox-
add-pm- utils-requireme nt branch. - 869. By Marc Tardif
-
Added support for TOUCH devices.
- 870. By Jeff Lane
-
Merged cr3s fix for bug #561816
- 871. By Marc Tardif
-
Merged from bladernr to add _attachment suffix and lsmod attachment.
- 872. By Marc Tardif
-
Updated pot file.
- 873. By Marc Tardif
-
Fixed memory persistence to be rooted like file persistence.
- 874. By Marc Tardif
-
Fixed persist module to support not being given a filename.
- 875. By Marc Tardif
-
Extended persist_info plugin to answer to both begin and prompt-begin messages.
- 876. By Marc Tardif
-
Added support for Python 2.5 in checkbox.
lib.transport. - 877. By Marc Tardif
-
Merged from audio_test_failing branch.
- 878. By Marc Tardif
-
Merged from syslog branch.
- 879. By Marc Tardif
-
Added stop signal when executing messages.
- 880. By Marc Tardif
-
Migrated UI from libglade to gtkbuilder which fixes bug #403534.
- 881. By Marc Tardif
-
Defining default options in checkbox.
application rather than CHECKBOX_OPTIONS environment variable. - 882. By Marc Tardif
-
Added changelog entry for candidate revision.
- 883. By Marc Tardif
-
Merged from 719552.
- 884. By Marc Tardif
-
Changed description of nautilus_file_copy job which fixes bug #709688.
- 885. By Marc Tardif
-
Fixed title in progress dialog.
- 886. By Marc Tardif
-
Updated changelog with new upstream release.
- 887. By Marc Tardif
-
Updated pot file.
- 888. By Marc Tardif
-
Merged from 727411.
- 889. By Marc Tardif
-
Merged from 691241.
- 890. By Marc Tardif
-
Added support for trying to submit twice which fixes bug #531010.
- 891. By Marc Tardif
-
Merged from 642001.
- 892. By Jeff Lane
-
Merged cli-cleanup for bug #221400
- 893. By Marc Tardif
-
Merged from checkbox-bug-fixes.
- 894. By Jeff Lane
-
Land translation work by Mahyuddin Susanto via Michael Terry
- 895. By Jeff Lane
-
Merged cr3 changes to changelog and control
- 896. By Marc Tardif
-
Added changelog entry for candidate revision.
- 897. By Jeff Lane
-
Commited Gerhard Burgers punctuation fix for LP #744167
- 898. By Marc Tardif
-
Merged from 553777.
- 899. By Marc Tardif
-
Moved Gerhard Burger in changelog from 0.11.2 to 0.11.3.
- 900. By Marc Tardif
-
Fixed missing capital letter in sleep_state_test description.
- 901. By Marc Tardif
-
Merged from 744964.
- 902. By Jeff Lane
-
Merged Marc Tardif fix for lp:729431
- 903. By Jeff Lane
-
Merged Marc Tardif fixes to hibernate test for lp:630785
- 904. By Jeff Lane
-
Merged addition of rtc test to sleep.txt.in to meet dependencies
- 905. By Marc Tardif
-
Reintroduced pm-utils requirements for jobs calling the sleep_test command and update po files.
- 906. By Marc Tardif
-
Merged checkbox log to apport report.
- 907. By Marc Tardif
-
Added changelog entry for candidate revision.
- 908. By Marc Tardif
-
Merged branch to enable camera detect test.
- 909. By Jeff Lane
-
Merged Carl Milettes fix for lp:507943 (disk_bench_test hard coded drive)
- 910. By Marc Tardif
-
Fixed eval of resources with names like list item names.
- 911. By Marc Tardif
-
Merged from pygi-gtk3-port.
- 912. By Marc Tardif
-
Removed dead pixel test.
- 913. By Marc Tardif
-
Merged from 773667.
- 914. By Marc Tardif
-
Merged from 776734.
- 915. By Marc Tardif
-
Merged from 786924.
- 916. By Marc Tardif
-
Merged from 776712.
- 917. By Jeff Lane
-
Applied Marc Tardifs changes to allow for remote submission (send results from a system other than the system under test)
- 918. By Marc Tardif
-
Merged from 588539.
- 919. By Marc Tardif
-
Merged from 665299.
- 920. By Marc Tardif
-
Wrapped submission stream to check for illegal xml characters.
- 921. By Marc Tardif
-
Replaced dependency on pytz with dateutil.
- 922. By Marc Tardif
-
Merged from 621880.
- 923. By Marc Tardif
-
Merged from add_camera_tests.
- 924. By Marc Tardif
-
Merged from fix_get_
pointer_ error. - 925. By Marc Tardif
-
Incremented version.
- 926. By Marc Tardif
-
Merged from 798200.
- 927. By Marc Tardif
-
Merged from patch_resourceo
bject_contains. - 928. By Marc Tardif
-
Merged from checkbox-
fix-pygi- misbehaviors. - 929. By Marc Tardif
-
Merged from 744190.
- 930. By Marc Tardif
-
Merged from add_interface_
option_ for_multi_ nic_test. - 931. By Marc Tardif
-
Merged from test-name-
in-report- a-bug. - 932. By Marc Tardif
-
Incremented version.
- 933. By Marc Tardif
-
Only reading CHECKBOX_* environment variables in config which fixes bug #802458.
Daniel Manrique (roadmr) wrote : | # |
Javier,
I have a question/comment about this branch.
The code works as advertised, with both whitelist and blacklist files, and both short (-W/-B) and long (--{black,
However it doesn't support the "extended" syntax :
checkbox-gtk --config=
If I call checkbox like this, I still get the old error but checkbox does continue executing the default set of tests without any warning to the user.
Is there a reason why you're checking for the file in application.py, as opposed to doing it in jobs_info.py? Perhaps we could fit the check at the point where jobs_info says "Failed to open file" and somehow flag that we need to stop execution at that point. I think it could potentially be cleaner that way. You know checkbox better than I do so I'll defer to your opinion in this point, but I would like to hear your thoughts on this.
This is a feature we in HW Cert were discussing would be useful, to avoid wasting time with bogus runs if the whitelist name is mis-specified so I'd like to get this to a point where we can merge it.
BTW the branch still merges cleanly against latest trunk.
Thanks and sorry for the delay in reviewing this code!
- 934. By Marc Tardif
-
Added preliminary default.whitelist.
- 935. By Marc Tardif
-
Imported scripts and jobs from Platform Services.
- 936. By Daniel Manrique
-
merge patch_log_
format_ object - 937. By Marc Tardif
-
Imported plugins from Platform Services.
- 938. By Daniel Manrique
-
merged patch_apport_
interface_ yesno - 939. By Marc Tardif
-
Merged switch to dh_python2 and debhelper7 which fixes bug #788514.
- 940. By Brendan Donegan
-
Removed some tests that shouldn't have appeared in the whitelist.
- 941. By Marc Tardif
-
Merged from improve_
failed_ test_message. - 942. By Daniel Manrique
-
merged Barry Warsaw's fix for problem with non-existing executables while building from clean tree
- 943. By Marc Tardif
-
Merged from camera_
test_grouping. - 944. By Marc Tardif
-
Merged from story325_
test_organisati on. - 945. By Marc Tardif
-
Updated pot file.
- 946. By Daniel Manrique
-
merged missing_suites branch
- 947. By Daniel Manrique
-
Fixed typo in local job definition which caused exception and test to not be run
- 948. By Daniel Manrique
-
merge story221_
move_30_ cycles_ to_stress - 949. By Daniel Manrique
-
merged fixup_powermana
gement_ suspends - 950. By Daniel Manrique
-
merged story221_
cpu_before_ after_suspend
Javier Collado (javier.collado) wrote : | # |
@Daniel
> However it doesn't support the "extended" syntax :
>
> checkbox-gtk
> --config=
Uhm, probably I didn't even try since I always use the shorthand options.
> Is there a reason why you're checking for the file in application.py, as
> opposed to doing it in jobs_info.py? Perhaps we could fit the check at the
> point where jobs_info says "Failed to open file" and somehow flag that we need
> to stop execution at that point. I think it could potentially be cleaner that
> way. You know checkbox better than I do so I'll defer to your opinion in this
> point, but I would like to hear your thoughts on this.
I think I wanted to print the error message as soon as possible to avoid initializing any of the interfaces. I'll double-check anyway.
> Thanks and sorry for the delay in reviewing this code!
Thank you for going through all the merge proposals.
Javier Collado (javier.collado) wrote : | # |
@Daniel
After trying to implement this patch in jobs_info plugin, I've found that it works providing the same user experience (gtk interface isn't initalized when the failure happens) and the problem regarding long configuration options is gone so I'll close this merge proposal as rejected and create a new one.
Thanks for your feedback.
Unmerged revisions
- 958. By Javier Collado
-
Print error message and exit when shorthand file (whitelist/
blacklist) isn't found
Preview Diff
1 | === modified file 'checkbox/application.py' |
2 | --- checkbox/application.py 2011-02-14 23:13:14 +0000 |
3 | +++ checkbox/application.py 2011-06-08 14:34:16 +0000 |
4 | @@ -91,6 +91,8 @@ |
5 | return parser.parse_args(args) |
6 | |
7 | def create_application(self, args=sys.argv): |
8 | + error_found = False |
9 | + |
10 | # Create data directory |
11 | data_directory = get_variable("CHECKBOX_DATA", ".") |
12 | safe_make_directory(data_directory) |
13 | @@ -100,6 +102,15 @@ |
14 | args[:0] = split(string_options) |
15 | (options, args) = self.parse_options(args) |
16 | |
17 | + # Check for shorthand file existence |
18 | + for shorthand_file_option in ("blacklist_file", "whitelist_file"): |
19 | + shorthand_file = getattr(options, shorthand_file_option) |
20 | + if shorthand_file and not posixpath.isfile(shorthand_file): |
21 | + sys.stderr.write(_("%s %s not found.\n") |
22 | + % (shorthand_file_option.capitalize().replace('_', ' '), |
23 | + repr(shorthand_file))) |
24 | + error_found = True |
25 | + |
26 | # Replace shorthands |
27 | for shorthand in "blacklist", "blacklist_file", "whitelist", "whitelist_file": |
28 | key = "checkbox/plugins/jobs_info/%s" % shorthand |
29 | @@ -113,6 +124,9 @@ |
30 | # Config setup |
31 | if len(args) != 2: |
32 | sys.stderr.write(_("Missing configuration file as argument.\n")) |
33 | + error_found = True |
34 | + |
35 | + if error_found: |
36 | sys.exit(1) |
37 | |
38 | config = Config() |
Please let me know if there's any problem with this merge proposal.
In my opinion it's good to integrate since it provides early information to the tester when a problem happens instead of omitting it.