Merge lp:~oem-qa/checkbox/patch_exit_on_shorthand_file_not_found into lp:checkbox

Proposed by Javier Collado
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
Reviewer Review Type Date Requested Status
Javier Collado (community) Disapprove
Daniel Manrique (community) Needs Information
Review via email: mp+41928@code.launchpad.net

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

To post a comment you must log in.
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.

Revision history for this message
Javier Collado (javier.collado) wrote :

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.

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

Revision history for this message
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,white}list-file) syntax.

However it doesn't support the "extended" syntax :

checkbox-gtk --config='checkbox/plugins/jobs_info/whitelist_file=bogus.whitelist'

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!

review: Needs Information
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_organisation.

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_powermanagement_suspends

950. By Daniel Manrique

merged story221_cpu_before_after_suspend

Revision history for this message
Javier Collado (javier.collado) wrote :

@Daniel

> However it doesn't support the "extended" syntax :
>
> checkbox-gtk
> --config='checkbox/plugins/jobs_info/whitelist_file=bogus.whitelist'

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.

Revision history for this message
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.

review: Disapprove

Unmerged revisions

958. By Javier Collado

Print error message and exit when shorthand file (whitelist/blacklist) isn't found

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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()

Subscribers

People subscribed via source and target branches