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)
Created attachment 9276 [details] [review] Bug 7994: scripts to check yaml files $ prove yaml_valid.t checks a closed list of files $ perl yaml_valid.pl -f myfile.yaml try to load a yaml file with YAML::LoadFile and show errors (both files pertidied)
$ 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
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.
Created attachment 9277 [details] [review] Bug 7994: fix sysprefs syntax yaml files
Created attachment 9278 [details] [review] Bug 7994: scripts to check yaml files $ prove yaml_valid.t checks a closed list of files $ perl yaml_valid.pl -f myfile.yaml try to load a yaml file with YAML::LoadFile and show errors (both files pertidied)
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?
Created attachment 10041 [details] [review] Fix indenting mistakes Fix error parse yaml files with $ perl yaml_valid.pl -f <file> After fixing all these mini mistakes I got: $ 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, 0 wallclock secs ( 0.04 usr 0.00 sys + 0.45 cusr 0.00 csys = 0.49 CPU) Result: PASS
Created attachment 10043 [details] [review] replace old patch
Created attachment 10044 [details] [review] Bug-7994-scripts-to-check-yaml-files.patch
Created attachment 10045 [details] [review] Bug-7994-Fix-indenting-mistakes.patch
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
Created attachment 10205 [details] [review] [SIGNED-OFF] Bug 7994: scripts to check yaml files $ prove yaml_valid.t checks a closed list of files $ perl yaml_valid.pl -f myfile.yaml try to load a yaml file with YAML::LoadFile and show errors (both files pertidied) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Yaml checker works. Second patch does not apply.
Created attachment 10207 [details] [review] Bug 7994: scripts to check yaml files $ prove yaml_valid.t checks a closed list of files $ perl yaml_valid.pl -f myfile.yaml try to load a yaml file with YAML::LoadFile and show errors (both files pertidied)
Created attachment 10208 [details] [review] Bug 7994: Fix pref yaml errors
rebased / fixed patch
Created attachment 10229 [details] [review] [SIGNED-OFF] Bug 7994: Fix pref yaml errors Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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.
Created attachment 10301 [details] [review] Bug 7994: scripts to check yaml files $ prove yaml_valid.t checks a closed list of files $ perl yaml_valid.pl -f myfile.yaml try to load a yaml file with YAML::LoadFile and show errors (both files pertidied) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 10302 [details] [review] [SIGNED-OFF] Bug 7994: scripts to check yaml files
Created attachment 10303 [details] [review] [SIGNED-OFF] Bug 7994: Fix pref yaml errors
(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 :)
(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 ?)
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
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
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
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.