Unit test input files are verified during `make verify-units-inputs` #1909

ahakanbaba · 2018-10-07T06:06:07Z

This is an early work in progress.

I am sending this PR out to discuss the implementation approach for incorporating the verification of the unit test input files to make check.

I have used some makefile metaprogramming. http://make.mad-scientist.net/the-eval-function/ I am by no means a makefile expert. So let me know please if there is a better approach to this.

Some missing pieces:

The CATEGORIES and UNITS env vars are not taken into consideration. So every time all puppet files are verified regardless.
The circileci config file needs to be updated to install puppet dependencies. The puppet command will fail in the containers.
The input.pp files have not been fixed to pass the puppet syntax checking.
If we want to go into this direction, the testing.mak should possibly be further parameterized or more meta-programming should be used. Right now the implementation is specific to the puppetManifest files.
Better variable names can be used in testing.mak

masatake · 2018-10-07T06:58:54Z

02188ce looks good to me but could you rename the file to README.md(?) or something reflecting the syntax of markup language.

0c50a74 is very interesting... I would like to take over this item. It should not be integrated to units target.

masatake · 2018-10-07T07:06:37Z

Oh, sorry, I should make a comment about 02188ce on #1908.

This is a work in progress.

We had to change some input.pp files to that they confirm to the puppet syntax. Due to those changes some tags have changed.

coveralls · 2018-10-10T20:21:58Z

Coverage remained the same at 84.887% when pulling 8181d5c on ahakanbaba:puppetManifest-verify-test-input-at-make-check into 1556085 on universal-ctags:master.

In th testing.mak, using a phony target will execute the target all the time. However for unit test input file checking, we only want to verify inputs that have been modified. An empty target construct in Makefile is suitable for that. Also update the gitignore file to ignore the empty target files.

ahakanbaba · 2018-10-10T21:03:04Z

Unfortunately having bmake build problems in fedora build

bmake: "/root/universal-ctags/Makefile" line 7323: Need an operator
bmake: "/root/universal-ctags/Makefile" line 7327: Need an operator
bmake: "/root/universal-ctags/Makefile" line 7329: Need an operator

The line of the makefile in the fedora container looks like this

7319                 --with-timeout=$(TIMEOUT)"; \
7320         $(SHELL) $${c} $(srcdir)/Units
7321
7322 #TODO: We shuold make this an empty target rather than a phony target
7323 define VERIFY_ONE_PUPPET_TEST_DIR
7324 $(1)/.input.pp.verified: $(1)/input.pp
7325         puppet apply --noop $$< && \
7326         touch $$@
7327 endef
7328
7329 $(foreach puppet_test_dir,$(PUPPET_TEST_DIRS),$(eval $(call VERIFY_ONE_PUPPET_TEST_DIR,$(puppet_test_dir))))
7330
7331 #
7332 # verify-units-inputs Target
7333 #

I would not be surprised if the bmake does not have the define (metaprogramming) feature . Do you know @masatake ?

ahakanbaba · 2018-10-10T22:18:12Z

@masatake This PR is about 70% there of what I had originally in mind.

The metaprogramming in the makefile needs to be further parameterized. Right now it has hardcoded strings with the puppetManifest.r in them . It should be possible to have only 1 more generic define block. Then generate input language checkers for many other languages with using only that block.
The build steps and the output of the make verify-units-inputs can be improved, maybe.
Also if you have better names, I am open to renaming.

Regardless, I think this PR is mature enough to be reviewed. Let me know if this approach is acceptable to you.

ahakanbaba · 2018-10-10T22:20:51Z

The puppet error messages fixed from the input.pp files are:

Error: Could not find node statement with name 'default' or 'mbp-005063.ad.whirl.net, mbp-005063.ad.whirl, mbp-005063.ad, mbp-005063' on node mbp-005063.ad.whirl.net
Error: Could not parse for environment production: The operator '+=' is no longer supported. See http://links.puppet.com/remove-plus-equals (file: /Users/hbaba/box/src/universal-ctags/ctags/Units/parser-puppetManifest.r/puppet-append.d/input.pp, line: 4, column: 10) on node mbp-005063.ad.whirl.net
Error: Could not parse for environment production: The parameter $name redefines a built in parameter in the 'define' expression (file: /Users/hbaba/box/src/universal-ctags/ctags/Units/parser-puppetManifest.r/puppet-fqdefinition.d/input.pp, line: 1, column: 25) on node mbp-005063.ad.whirl.net
Error: Class 'one' is already defined (file: /Users/hbaba/box/src/universal-ctags/ctags/Units/parser-puppetManifest.r/puppet-multipleclass.d/input.pp, line: 1); cannot redefine (file: /Users/hbaba/box/src/universal-ctags/ctags/Units/parser-puppetManifest.r/puppet-multipleclass.d/input.pp, line: 5) on node mbp-005063.ad.whirl.net
Error: Parameter mode failed on File[/tmp/scopetest]: The file mode specification must be a string, not 'Integer' (file: /Users/hbaba/box/src/universal-ctags/ctags/Units/parser-puppetManifest.r/puppet-scopetest.d/input.pp, line: 5)

ahakanbaba · 2018-10-10T22:23:26Z

makefiles/testing.mak

+    $(shell dirname $(path)))
+VERIFY_PUPPET_TEST_DIRS_TARGETS := $(PUPPET_TEST_DIRS:%=%/.input.pp.verified)
+
+#TODO: We shuold make this an empty target rather than a phony target


This comment is not applicable anymore. Should be deleted

b4n · 2018-10-11T06:57:23Z

makefiles/testing.mak

@@ -88,6 +88,31 @@ slap: $(CTAGS_TEST)
 		--with-timeout=$(TIMEOUT)"; \
 	$(SHELL) $${c} $(srcdir)/Units

+# TODO: Possibly the bmake does not support the metaprogramming similar to gnu make.


I'd rather avoid using non-portable Make stuff if possible, assuming the rest of our makefiles work with BSD make. Here it seems fairly easy to simply make it the content of the target itself, it doesn't seem very important to build target, nor even only build once -- rebuilding seems unlikely and innocent enough.

So, I guess I'd just do something like this (not tested at all, just for the argument):

verify-units-inputs: verify-units-inputs-puppet verify-units-inputs-puppet: find $(srcdir)/Units/parser-puppetManifest.r -name expected.tags | $(SED) 's%/[^/]*$$%%' | while read -r dir; do \ puppet apply --noop "$$dir/input.pp" || exit 1; \ done

I see couple of more problems with this approach in the comment.

One cannot use the make job control mode (-j) for verify-units-inputs. All input.XX file verification has to be done serially.

What @b4n mentions: Cannot avoid re-execution. What I have seen, re-execution can be quite expensive. The input language toolchain does not necessarily have fast startup time. In this change, puppet runs take several minutes if executed serially. Consider the need to run interpreters, compilers for each input.xx file. Hence, for fast test and development time both (1) and (2) are important.

The complexity of the testing.mak is bound to grow. ctags supports 10's of different input languages. Each having their verify-untis-inputs-<language_name> target is not easy to maintain. Compared to that, the conditional of if !USING_BMAKE could be acceptable.

These three are important drawbacks.

With using metaprogramming, we gain on all 3 of them. (See my next push to this PR) . What we lose is missing verify-units-inputs target in BSD builds. That will not prevent BSD platforms from building and using ctags. Developers on BSD only won't be able to execute make verify-units-intput locally, if they are submitting PRs from BSD platforms.

I have not done development on BSD before. But that does not seem like a big blocker either https://unix.stackexchange.com/a/127955/212862

Fair enough, and you're right that this is probably not a critical enough part of the build system that it can't have its own requirements. Also, we can remember that GNU make is fairly portable by itself, and although not everybody likes it, it is usually possible to get it on most systems; AFAIK it's available on all BSDs under gmake, yet probably not in a stock installation.

Anyway, given your arguments I only have one concern now: we should check for GNU make, not against BSD make. There are many Make implementations that aren't compatible with GNU make, and most Make implementations don't have those metaprogramming features. So unless somebody feels like tesing every single Make implementation around and see what exact subset it supports, I strongly suggests we have a whitelist instead of a blacklist, and only have GNU make in it at first.

PS: moreover, most Make implementations are installed as make, not as other aliases like bmake, gmake, pmake or whatnot. Checking for the name of $MAKE is not a solid way of cheking which Make implementation is in use.

Thanks for the input. I agree on both aspects. The whitelist-blacklist and checking the name of $MAKE. I will hold off on the implementation, though. @masatake mentioned that he will implement this in /misc/units script. If anything changes, I am more than willing to address your comments in this PR.

The makefile now uses two nested defines to fully parameterize the unit test input file parameterization.

1) Consume the stdout of the verification command. Prints too much to the console. Useful information to debug will be printed via the makefile echo and also the std err anyways 2) Add a clean-verify-units-input command.

Set the clean dependencies. Get the extension parameter and use it correctly in the define block Install further test requirements in centos container.

jq comes from epel-release.

ahakanbaba · 2018-10-11T23:06:48Z

In the latest version of this PR (16 commits) we enable input language checking while

Supporting parallel execution with job control mode in make (-j).
Verifying only the input.XX files that have changed since the last execution of make verify-units-inputs
Start testing new input languages with only a one line change similar to this

With this PR, we check not only the puppet files but also the json files.

ahakanbaba · 2018-10-15T02:38:15Z

@masatake @BN4
Any further comments on this ?

masatake · 2018-10-15T02:55:15Z

I would like to move the logic to misc/units though it makes running parallel impossible.
I will work on this item as the next task. I need this feature for #1847.
NewJava parsre expects the input is syntatically correct. However, the test cases under parsers-java.r are not. I need the infrastructure veryfing input for Java.

I will borrow many codes from this pull request. However, I will design the infrastructure very differently based on my experience implementing misc/units, the backend of make units.

ahakanbaba · 2018-10-15T03:40:07Z

Let me know if you need any help @masatake.

In the meantime: It may be throwaway but the single line to support java tests in this testing.mak looks like this:

masatake · 2018-10-16T16:18:28Z

Let me know if you need any help @masatake.
Great offering!

I always need help for documentation. Especially #1737.
However, I'm not sure whether you are interested in topics other than PuppetManifest.
If you are interested in improving whole ctags, I would like to know your area of interest and area you are familiar with.

All unclosed issues you opend will take time to solve. All the items are cross-parser issues. (Therefore, they are important.)

ahakanbaba · 2018-10-17T21:46:13Z

Let me know if you need any help @masatake.
Great offering!

I always need help for documentation. Especially #1737.
However, I'm not sure whether you are interested in topics other than PuppetManifest.
If you are interested in improving whole ctags, I would like to know your area of interest and area you are familiar with.

All unclosed issues you opend will take time to solve. All the items are cross-parser issues. (Therefore, they are important.)

@masatake my biggest priority now is to improve ctags experience with puppet code. I have opened several tickets that should improve exactly that. I would be very interested to help with that purpose.

Unfortunately, I do not think I have the bandwidth to contribute to many other exciting areas in ctags.

masatake · 2018-10-18T02:37:37Z

@ahakanbaba, I see.

@ahakanbaba

Conceptualy derived from a pull request, universal-ctags#1909 sent by @ahakanbaba. A example session: $ bash misc/units verify-input Units misc/verifiers .. Category: ROOT ------------------------------------------------------------ simple-json.d/input.json with jq valid zephir-simple.d/input.zep with zoop unavailable Summary ------------------------------------------------------------ #valid: 44 #invalid: 19 #skipped (known invalidation) 1 #skipped (verifier unavailable) 1 Unavailable verifiers ------------------------------------------------------------ zoop You can specify verfiiers to use with --verifiers= option. $ dash misc/units verify-input --verifiers=jq Units misc/verifiers .. Usage: A test case developer puts "verfier" file to one's test case directory or one's category directory. Put a name of verifier to the file. If the file is at a category directory (Units/*.r), the verifier specified in a file is used for verifying all input files under the category. Put "verify" file in a test case directory to override the verifier specified in the category directory. "KNOWN-INVALIDATION" is a special verifier. If the input is kept invalid intentionally, specify the verifier. A verifier developer writes verifier-foo file and puts it to misc/verifiers. Shell is suitable for writing the file. The execution bit of the file must be set. Here, "foo" is the name of verifier which may be specified in "verfier" files under Units. The file name is made a bit redundant. misc/units runs a verifier command in two different modes. If "is_runnable" subcommand is given as the first argument, return 0 if the verifier is ready to run. A verifier may depend on external tools. If the verifier cannot find the external tools in the runtime environment, the verifier exits with non-zero. misc/units never runs the verifier if the verifier exits with non-zero for the subcommand. If "verify" subcommand is given as the frist argument, the script do verify the input file passed as the second argument. It returns 0 if the input is valid, or non-zero if it is invalid. TODO: The above memo should be written in docs/testing.rst. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

verify-input target derrived from #1909

@ahakanbaba

Units/parser-puppetManifest.r/puppet-append.d/verifier is very special. About puppet-append.d/verifier, the puppet verifier reports this input as "invalid" because `+=' operator is obsoleted in puppet5. @ahakanbaba in universal-ctags#1909 reported and proposed a fix for this already. However, @masatake kept this as is for following reasons: - valid in puppet4, - testing KNOWN-INVALIDATION special verifier, and - testing '#' comment in a verifier file. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

@ahakanbaba

Units/parser-puppetManifest.r/puppet-append.d/verifier is very special. About puppet-append.d/verifier, the puppet verifier reports this input as "invalid" because `+=' operator is obsoleted in puppet5. @ahakanbaba in universal-ctags#1909 reported and proposed a fix for this already. However, @masatake kept this as is for following reasons: - valid in puppet4, - testing KNOWN-INVALIDATION special verifier, and - testing '#' comment in a verifier file. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

ahakanbaba · 2018-10-29T05:50:47Z

Closing in favor of #1921

@ahakanbaba

Units/parser-puppetManifest.r/puppet-append.d/validator is very special. About puppet-append.d/validator, the puppet validator reports this input as "invalid" because `+=' operator is obsoleted in puppet5. @ahakanbaba in universal-ctags#1909 reported and proposed a fix for this already. However, @masatake kept this as is for following reasons: - valid in puppet4, - testing KNOWN-INVALIDATION special validator, and - testing '#' comment in a validator file. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

Introduce puppet verifier (derived from #1909)

masatake self-assigned this Oct 7, 2018

ahakanbaba force-pushed the puppetManifest-verify-test-input-at-make-check branch from d1c82b9 to 0eb7f59 Compare October 7, 2018 17:32

ahakanbaba added 3 commits October 9, 2018 23:38

Attempt to incorporate unit test input file checking to make check

01bf87a

This is a work in progress.

Verify input file only expected.tags exists.

68aa5e8

Fixing puppet syntax and semantic errors in input.pp files

a5eb81e

ahakanbaba force-pushed the puppetManifest-verify-test-input-at-make-check branch from 6210e77 to a5eb81e Compare October 10, 2018 06:38

After fixing puppet files, fix the expected.tags files.

ac54a3f

We had to change some input.pp files to that they confirm to the puppet syntax. Due to those changes some tags have changed.

ahakanbaba added 5 commits October 10, 2018 14:10

Do not use metaprogramming in Makefile if using bmake

26a3a48

Disable metaprogramming with autoconf/make variable

7619745

Install puppet to centos tests.

1652bf4

Enable verify-units-inputs check in centos tests.

0553dc2

Set the path env var at the right place

b31ca7f

ahakanbaba commented Oct 10, 2018

View reviewed changes

b4n reviewed Oct 11, 2018

View reviewed changes

ahakanbaba added 6 commits October 11, 2018 14:35

First working attempt of further parametirization of verify-test-inputs.

e339223

The makefile now uses two nested defines to fully parameterize the unit test input file parameterization.

Minor improvements to the makefile metaprogramming

f7132f7

1) Consume the stdout of the verification command. Prints too much to the console. Useful information to debug will be printed via the makefile echo and also the std err anyways 2) Add a clean-verify-units-input command.

In verify-units-inputs also parameterize the command

8b383e7

Further metaprogramming improvements.

234540f

Set the clean dependencies. Get the extension parameter and use it correctly in the define block Install further test requirements in centos container.

Force rm in the clean target

ba44ae3

Install epel release first jq later.

8181d5c

jq comes from epel-release.

ahakanbaba changed the title ~~Unit test input files are verified during make check ( For now only puppetManifest files)~~ Unit test input files are verified during make check Oct 11, 2018

ahakanbaba changed the title ~~Unit test input files are verified during make check~~ Unit test input files are verified during make verify-units-inputs Oct 11, 2018

masatake mentioned this pull request Oct 19, 2018

verify-input target derrived from #1909 #1921

Merged

masatake added a commit that referenced this pull request Oct 23, 2018

Merge pull request #1921 from masatake/verify-input-base

59c2a32

verify-input target derrived from #1909

ahakanbaba closed this Oct 29, 2018

ahakanbaba mentioned this pull request Nov 7, 2018

Introduce puppet verifier (derived from #1909) #1926

Merged

masatake added a commit that referenced this pull request Nov 7, 2018

Merge pull request #1926 from masatake/verify-input-pr

befcb88

Introduce puppet verifier (derived from #1909)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test input files are verified during `make verify-units-inputs` #1909

Unit test input files are verified during `make verify-units-inputs` #1909

ahakanbaba commented Oct 7, 2018

masatake commented Oct 7, 2018

masatake commented Oct 7, 2018

coveralls commented Oct 10, 2018 •

edited

Loading

ahakanbaba commented Oct 10, 2018 •

edited

Loading

ahakanbaba commented Oct 10, 2018

ahakanbaba commented Oct 10, 2018 •

edited

Loading

ahakanbaba Oct 10, 2018

b4n Oct 11, 2018

ahakanbaba Oct 11, 2018 •

edited

Loading

b4n Oct 15, 2018

b4n Oct 15, 2018

ahakanbaba Oct 15, 2018

ahakanbaba commented Oct 11, 2018

ahakanbaba commented Oct 15, 2018

masatake commented Oct 15, 2018

ahakanbaba commented Oct 15, 2018

masatake commented Oct 16, 2018

ahakanbaba commented Oct 17, 2018

masatake commented Oct 18, 2018

ahakanbaba commented Oct 29, 2018

Unit test input files are verified during make verify-units-inputs #1909

Unit test input files are verified during make verify-units-inputs #1909

Conversation

ahakanbaba commented Oct 7, 2018

masatake commented Oct 7, 2018

masatake commented Oct 7, 2018

coveralls commented Oct 10, 2018 • edited Loading

ahakanbaba commented Oct 10, 2018 • edited Loading

ahakanbaba commented Oct 10, 2018

ahakanbaba commented Oct 10, 2018 • edited Loading

ahakanbaba Oct 10, 2018

Choose a reason for hiding this comment

b4n Oct 11, 2018

Choose a reason for hiding this comment

ahakanbaba Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

b4n Oct 15, 2018

Choose a reason for hiding this comment

b4n Oct 15, 2018

Choose a reason for hiding this comment

ahakanbaba Oct 15, 2018

Choose a reason for hiding this comment

ahakanbaba commented Oct 11, 2018

ahakanbaba commented Oct 15, 2018

masatake commented Oct 15, 2018

ahakanbaba commented Oct 15, 2018

masatake commented Oct 16, 2018

ahakanbaba commented Oct 17, 2018

masatake commented Oct 18, 2018

ahakanbaba commented Oct 29, 2018

Unit test input files are verified during `make verify-units-inputs` #1909

Unit test input files are verified during `make verify-units-inputs` #1909

coveralls commented Oct 10, 2018 •

edited

Loading

ahakanbaba commented Oct 10, 2018 •

edited

Loading

ahakanbaba commented Oct 10, 2018 •

edited

Loading

ahakanbaba Oct 11, 2018 •

edited

Loading