Bug 7994 - Syntax error in yaml (syspref) files
Summary: Syntax error in yaml (syspref) files
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low trivial (vote)
Target Milestone: ---
Assignee: claire.hernandez@biblibre.com
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 10:09 UTC by Paul Poulain
Modified: 2013-12-05 19:59 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7994: scripts to check yaml files (5.23 KB, patch)
2012-04-23 13:17 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug 7994: fix sysprefs syntax yaml files (5.49 KB, patch)
2012-04-23 13:39 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug 7994: scripts to check yaml files (5.30 KB, patch)
2012-04-23 13:40 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Fix indenting mistakes (5.81 KB, patch)
2012-06-09 15:29 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
replace old patch (3.91 KB, patch)
2012-06-09 15:34 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug-7994-scripts-to-check-yaml-files.patch (3.91 KB, patch)
2012-06-09 15:34 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug-7994-Fix-indenting-mistakes.patch (5.82 KB, patch)
2012-06-09 15:35 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7994: scripts to check yaml files (3.98 KB, patch)
2012-06-11 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7994: scripts to check yaml files (3.91 KB, patch)
2012-06-11 13:54 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug 7994: Fix pref yaml errors (5.48 KB, patch)
2012-06-11 13:56 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7994: Fix pref yaml errors (5.54 KB, patch)
2012-06-11 15:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7994: scripts to check yaml files (4.04 KB, patch)
2012-06-15 15:18 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7994: scripts to check yaml files (4.05 KB, patch)
2012-06-15 15:20 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7994: Fix pref yaml errors (5.54 KB, patch)
2012-06-15 15:20 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-04-23 10:09:41 UTC
There is a syntax error in YAML files used for sysprefs.

When you try to run a translation, you get a nasty:
Syck parser (line 53, column 0): syntax error at /usr/lib/perl5/YAML/Syck.pm line 76, <$fh> line 1.


We should probably add a test for yaml validity using Test::YAML::Valid perl package

(BibLibre mantis 8667)
Comment 1 claire.hernandez@biblibre.com 2012-04-23 13:17:00 UTC Comment hidden (obsolete)
Comment 2 claire.hernandez@biblibre.com 2012-04-23 13:20:13 UTC
$ prove yaml_valid.t 
yaml_valid.t .. 1/1 
#   Failed test '../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref is YAML'
#   at yaml_valid.t line 48.

#   Failed test '../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref is YAML'
#   at yaml_valid.t line 48.

#   Failed test '../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref is YAML'
#   at yaml_valid.t line 48.
yaml_valid.t .. 18/1 # Looks like you planned 1 test but ran 19.
# Looks like you failed 3 tests of 19 run.
yaml_valid.t .. Dubious, test returned 3 (wstat 768, 0x300)
All 1 subtests passed 

Test Summary Report
-------------------
yaml_valid.t (Wstat: 768 Tests: 19 Failed: 18)
  Failed tests:  2-19
  Non-zero exit status: 3
  Parse errors: Bad plan.  You planned 1 tests but ran 19.
Files=1, Tests=19,  1 wallclock secs ( 0.04 usr  0.00 sys +  0.53 cusr  0.02 csys =  0.59 CPU)
Result: FAIL


Should be fixed:


15:17 ~/dev/versions/community/xt (bug_7994 $)$ perl yaml_valid.pl -f ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref
Testing file: ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref
KO!
YAML Error: Invalid element in map
   Code: YAML_LOAD_ERR_BAD_MAP_ELEMENT
   Line: 4
   Document: 1
 at /usr/local/share/perl/5.10.1/YAML/Loader.pm line 352

15:17 ~/dev/versions/community/xt (bug_7994 $)$ perl yaml_valid.pl -f ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref 
Testing file: ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
KO!
YAML Error: Expected separator '---'
   Code: YAML_PARSE_ERR_NO_SEPARATOR
   Line: 83
   Document: 2
 at /usr/local/share/perl/5.10.1/YAML/Loader.pm line 81

15:19 ~/dev/versions/community/xt (bug_7994 $)$ perl yaml_valid.pl -f ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref 
Testing file: ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref
KO!
YAML Error: Invalid element in map
   Code: YAML_LOAD_ERR_BAD_MAP_ELEMENT
   Line: 10
   Document: 1
 at /usr/local/share/perl/5.10.1/YAML/Loader.pm line 352
Comment 3 claire.hernandez@biblibre.com 2012-04-23 13:37:39 UTC
I don't see syck parser error in community master today:
I just have a:
./translate install fr-FR
Can't use string ("b0VIM 7.3") as a HASH ref while "strict refs" in use at LangInstaller.pm line 194.
Comment 4 claire.hernandez@biblibre.com 2012-04-23 13:39:08 UTC Comment hidden (obsolete)
Comment 5 claire.hernandez@biblibre.com 2012-04-23 13:40:15 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2012-05-13 14:19:23 UTC
I just tried applying these patches and all 19 tests fail, even after I install Test::YAML::Valid. I get the same error for every single one:

#   Failed test '../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/web_services.pref is YAML'
#   at xt/yaml_valid.t line 50.
# Looks like you failed 19 tests of 19.

Shouldn't the tests pass?
Comment 7 claire.hernandez@biblibre.com 2012-06-09 15:29:08 UTC Comment hidden (obsolete)
Comment 8 claire.hernandez@biblibre.com 2012-06-09 15:34:04 UTC Comment hidden (obsolete)
Comment 9 claire.hernandez@biblibre.com 2012-06-09 15:34:52 UTC Comment hidden (obsolete)
Comment 10 claire.hernandez@biblibre.com 2012-06-09 15:35:23 UTC Comment hidden (obsolete)
Comment 11 claire.hernandez@biblibre.com 2012-06-09 15:38:21 UTC
Should be better:

$ git bz apply 7994
...

$ cd xt

$ prove yaml_valid.t 
yaml_valid.t .. ../koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences at yaml_valid.t line 27.
yaml_valid.t .. ok     
All tests successful.
Files=1, Tests=16,  1 wallclock secs ( 0.04 usr  0.01 sys +  0.46 cusr  0.01 csys =  0.52 CPU)
Result: PASS
Comment 12 Kyle M Hall 2012-06-11 13:48:26 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2012-06-11 13:49:29 UTC
Yaml checker works. Second patch does not apply.
Comment 14 claire.hernandez@biblibre.com 2012-06-11 13:54:10 UTC Comment hidden (obsolete)
Comment 15 claire.hernandez@biblibre.com 2012-06-11 13:56:42 UTC Comment hidden (obsolete)
Comment 16 claire.hernandez@biblibre.com 2012-06-11 14:13:23 UTC
rebased / fixed patch
Comment 17 Kyle M Hall 2012-06-11 15:25:00 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2012-06-12 09:27:34 UTC
QA Comments:
1/ Usage is not correct:
+=head1 USAGE
+
+From Koha root directory:
+
+prove -v xt/tt_valid.t

2/ It not possible to launch the test file from the koha home directory (prove xt/yaml_valid.t) (see the FindBin module)

3/ Don't you think it would be more friendly to do a test file (thus using Test::More) for your pl file ?

4/ You add a new dependency Test::YAML::Valid. I dont know if we have to add this dependency to koha (given that it is used for unit tests).

Marking as Failed QA.
Comment 19 claire.hernandez@biblibre.com 2012-06-15 15:18:48 UTC Comment hidden (obsolete)
Comment 20 claire.hernandez@biblibre.com 2012-06-15 15:20:26 UTC
Created attachment 10302 [details] [review]
[SIGNED-OFF] Bug 7994: scripts to check yaml files
Comment 21 claire.hernandez@biblibre.com 2012-06-15 15:20:59 UTC
Created attachment 10303 [details] [review]
[SIGNED-OFF] Bug 7994: Fix pref yaml errors
Comment 22 claire.hernandez@biblibre.com 2012-06-15 15:24:29 UTC
(In reply to comment #18)

> 1/ Usage is not correct:
> +=head1 USAGE
> +
> +From Koha root directory:
> +
> +prove -v xt/tt_valid.t

Yep done.

> 2/ It not possible to launch the test file from the koha home directory
> (prove xt/yaml_valid.t) (see the FindBin module) 

Done but too much code for me for adding this "feature" - nothing more sexy?

> 3/ Don't you think it would be more friendly to do a test file (thus using
> Test::More) for your pl file ?

I don't know how to give a parameter to a test file and I wanted to have a "-f" option to test whatever you want (sudoc yaml file for example).
 
> 4/ You add a new dependency Test::YAML::Valid. I dont know if we have to add
> this dependency to koha (given that it is used for unit tests).

I think we should be free to add new test libraies if we need it for testing. For discusion irl monday :)
Comment 23 Paul Poulain 2012-06-15 15:41:29 UTC
(In reply to comment #22)
> > 4/ You add a new dependency Test::YAML::Valid. I dont know if we have to add
> > this dependency to koha (given that it is used for unit tests).
> 
> I think we should be free to add new test libraies if we need it for
> testing. For discusion irl monday :)

I agree with claire = it's not a Koha dependency, it's a jenkins one, I think it's OK not to add it to Koha

(claire = shouldn't you switch back to signed-off ?)
Comment 24 Chris Cormack 2012-06-16 10:37:18 UTC
I agree with Paul, this is a test patch ... someone wrote a test!! we should be giving them flowers not making it harder for it to get into Koha
Comment 25 Marcel de Rooy 2012-06-18 07:12:53 UTC
QA Comment:
Some points have received attention.
The new dependency should be marked optional. Please add it in a followup.
The suggested additional options could be handled similarly.

Passed QA
Comment 26 Paul Poulain 2012-06-20 19:10:47 UTC
Patch pushed, and I don't think a follow-up is needed to add the dependency, because it's unneeded for a standard production server, ppl installing will be wondering why and if they should install this package
Comment 27 Chris Cormack 2012-06-23 07:15:51 UTC
Pushed to 3.8.x, virtual flowers to Claire for not only fixing the error but writing a test script so errors won't creep back in.