Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 16 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
01bf87a
Attempt to incorporate unit test input file checking to `make check`
ahakanbaba Oct 7, 2018
68aa5e8
Verify input file only expected.tags exists.
ahakanbaba Oct 8, 2018
a5eb81e
Fixing puppet syntax and semantic errors in input.pp files
ahakanbaba Oct 10, 2018
ac54a3f
After fixing puppet files, fix the expected.tags files.
ahakanbaba Oct 10, 2018
46276ca
Use an empty target instead of a phony target
ahakanbaba Oct 10, 2018
26a3a48
Do not use metaprogramming in Makefile if using bmake
ahakanbaba Oct 10, 2018
7619745
Disable metaprogramming with autoconf/make variable
ahakanbaba Oct 10, 2018
1652bf4
Install puppet to centos tests.
ahakanbaba Oct 10, 2018
0553dc2
Enable verify-units-inputs check in centos tests.
ahakanbaba Oct 10, 2018
b31ca7f
Set the path env var at the right place
ahakanbaba Oct 10, 2018
e339223
First working attempt of further parametirization of verify-test-inputs.
ahakanbaba Oct 11, 2018
f7132f7
Minor improvements to the makefile metaprogramming
ahakanbaba Oct 11, 2018
8b383e7
In verify-units-inputs also parameterize the command
ahakanbaba Oct 11, 2018
234540f
Further metaprogramming improvements.
ahakanbaba Oct 11, 2018
ba44ae3
Force rm in the clean target
ahakanbaba Oct 11, 2018
8181d5c
Install epel release first jq later.
ahakanbaba Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
*.ssexwsp.user
*.TMP
*.tmp
# Makefile empty target files of verify-units-inputs target
Units/**/.input.*.verified
*~
.deps
.dirstamp
Expand Down
7 changes: 4 additions & 3 deletions Units/parser-puppetManifest.r/node.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
www1.example.com input.pp /^node 'www1.example.com' {$/;" node line:2 language:PuppetManifest
www2.example.com input.pp /^node 'www2.example.com', 'www3.example.com' {$/;" node line:12 language:PuppetManifest
www3.example.com input.pp /^node 'www2.example.com', 'www3.example.com' {$/;" node line:12 language:PuppetManifest
default input.pp /^node 'default' {}$/;" node line:2 language:PuppetManifest
www1.example.com input.pp /^node 'www1.example.com' {$/;" node line:3 language:PuppetManifest
www2.example.com input.pp /^node 'www2.example.com', 'www3.example.com' {$/;" node line:13 language:PuppetManifest
www3.example.com input.pp /^node 'www2.example.com', 'www3.example.com' {$/;" node line:13 language:PuppetManifest
1 change: 1 addition & 0 deletions Units/parser-puppetManifest.r/node.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* Taken from https://docs.puppet.com/puppet/5.1/lang_node_definitions.html */
node 'default' {}
node 'www1.example.com' {
include common
include apache
Expand Down
1 change: 1 addition & 0 deletions Units/parser-puppetManifest.r/puppet-append.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
var input.pp /^$var=['\/tmp\/file1','\/tmp\/file2']$/;" variable line:1 language:PuppetManifest
arraytest input.pp /^class arraytest {$/;" class line:3 language:PuppetManifest end:9
var input.pp /^ $var = $var + ['\/tmp\/file3', '\/tmp\/file4']$/;" variable line:4 language:PuppetManifest scope:class:arraytest
2 changes: 1 addition & 1 deletion Units/parser-puppetManifest.r/puppet-append.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
$var=['/tmp/file1','/tmp/file2']

class arraytest {
$var += ['/tmp/file3', '/tmp/file4']
$var = $var + ['/tmp/file3', '/tmp/file4']
file {
$var:
content => "test"
Expand Down
4 changes: 2 additions & 2 deletions Units/parser-puppetManifest.r/puppet-collection_within_virtual_definitions.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
test input.pp /^define test($name) {$/;" definition line:1 language:PuppetManifest end:6
/tmp/collection_within_virtual_definitions1_$name.txt input.pp /^ file {"\/tmp\/collection_within_virtual_definitions1_$name.txt":$/;" resource line:2 language:PuppetManifest scope:definition:test end:4
test input.pp /^define test($my_name) {$/;" definition line:1 language:PuppetManifest end:6
/tmp/collection_within_virtual_definitions1_$my_name.txt input.pp /^ file {"\/tmp\/collection_within_virtual_definitions1_$my_name.txt":$/;" resource line:2 language:PuppetManifest scope:definition:test end:4
test2 input.pp /^define test2() {$/;" definition line:8 language:PuppetManifest end:12
/tmp/collection_within_virtual_definitions2_$name.txt input.pp /^ file {"\/tmp\/collection_within_virtual_definitions2_$name.txt":$/;" resource line:9 language:PuppetManifest scope:definition:test2 end:11
foo input.pp /^ @test {"foo":$/;" resource line:15 language:PuppetManifest end:17
Expand Down
8 changes: 4 additions & 4 deletions Units/parser-puppetManifest.r/puppet-collection_within_virtual_definitions.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
define test($name) {
file {"/tmp/collection_within_virtual_definitions1_$name.txt":
content => "File name $name\n"
define test($my_name) {
file {"/tmp/collection_within_virtual_definitions1_$my_name.txt":
content => "File name $my_name\n"
}
Test2 <||>
}
Expand All @@ -13,7 +13,7 @@

node default {
@test {"foo":
name => "foo"
my_name => "foo"
}
@test2 {"foo2": }
Test <||>
Expand Down
2 changes: 2 additions & 0 deletions Units/parser-puppetManifest.r/puppet-emptyif.d/args.ctags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
--sort=no
--fields=+KZlne
Empty file added Units/parser-puppetManifest.r/puppet-emptyif.d/expected.tags
View file
Open in desktop
Empty file.
4 changes: 4 additions & 0 deletions Units/parser-puppetManifest.r/puppet-emptyif.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

if true {
# still nothing
}
3 changes: 0 additions & 3 deletions Units/parser-puppetManifest.r/puppet-emptyifelse.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,3 @@
# nothing here
}

if true {
# still nothing
}
4 changes: 2 additions & 2 deletions Units/parser-puppetManifest.r/puppet-multipleclass.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
one input.pp /^class one {$/;" class line:1 language:PuppetManifest end:3
/tmp/multipleclassone input.pp /^ file { "\/tmp\/multipleclassone": content => "one" }$/;" resource line:2 language:PuppetManifest scope:class:one end:2
one input.pp /^class one {$/;" class line:5 language:PuppetManifest end:7
/tmp/multipleclasstwo input.pp /^ file { "\/tmp\/multipleclasstwo": content => "two" }$/;" resource line:6 language:PuppetManifest scope:class:one end:6
two input.pp /^class two {$/;" class line:5 language:PuppetManifest end:7
/tmp/multipleclasstwo input.pp /^ file { "\/tmp\/multipleclasstwo": content => "two" }$/;" resource line:6 language:PuppetManifest scope:class:two end:6
2 changes: 1 addition & 1 deletion Units/parser-puppetManifest.r/puppet-multipleclass.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
file { "/tmp/multipleclassone": content => "one" }
}

class one {
class two {
file { "/tmp/multipleclasstwo": content => "two" }
}

Expand Down
4 changes: 2 additions & 2 deletions Units/parser-puppetManifest.r/puppet-scopetest.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
mode input.pp /^$mode = 640$/;" variable line:2 language:PuppetManifest
mode input.pp /^$mode = "640"$/;" variable line:2 language:PuppetManifest
thing input.pp /^define thing {$/;" definition line:4 language:PuppetManifest end:6
/tmp/$name input.pp /^ file { "\/tmp\/$name": ensure => file, mode => $mode }$/;" resource line:5 language:PuppetManifest scope:definition:thing end:5
testing input.pp /^class testing {$/;" class line:8 language:PuppetManifest end:11
mode input.pp /^ $mode = 755$/;" variable line:9 language:PuppetManifest scope:class:testing
mode input.pp /^ $mode = "755"$/;" variable line:9 language:PuppetManifest scope:class:testing
4 changes: 2 additions & 2 deletions Units/parser-puppetManifest.r/puppet-scopetest.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@

$mode = 640
$mode = "640"

define thing {
file { "/tmp/$name": ensure => file, mode => $mode }
}

class testing {
$mode = 755
$mode = "755"
thing {scopetest: }
}

Expand Down
26 changes: 13 additions & 13 deletions Units/parser-puppetManifest.r/puppet-selectorvalues.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,37 +6,37 @@
$test = "yay"

$mode1 = $value1 ? {
"" => 755,
default => 644
"" => "755",
default => "644"
}

$mode2 = $value2 ? {
true => 755,
default => 644
true => "755",
default => "644"
}

$mode3 = $value3 ? {
false => 755,
default => 644
false => "755",
default => "644"
}

$mode4 = $value4 ? {
$test => 755,
default => 644
$test => "755",
default => "644"
}

$mode5 = yay ? {
$test => 755,
default => 644
$test => "755",
default => "644"
}

$mode6 = $mode5 ? {
755 => 755
"755" => "755"
}

$mode7 = "test regex" ? {
/regex$/ => 755,
default => 644
/regex$/ => "755",
default => "644"
}


Expand Down
2 changes: 1 addition & 1 deletion Units/parser-puppetManifest.r/puppet-simpleselector.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ var input.pp /^$var = "value"$/;" variable line:3 language:PuppetManifest
/tmp/snippetselectbtest input.pp /^file { "\/tmp\/snippetselectbtest":$/;" resource line:13 language:PuppetManifest end:19
othervar input.pp /^$othervar = "complex value"$/;" variable line:21 language:PuppetManifest
/tmp/snippetselectctest input.pp /^file { "\/tmp\/snippetselectctest":$/;" resource line:23 language:PuppetManifest end:29
anothervar input.pp /^$anothervar = Yayness$/;" variable line:30 language:PuppetManifest
anothervar input.pp /^$anothervar = "Yayness"$/;" variable line:30 language:PuppetManifest
/tmp/snippetselectdtest input.pp /^file { "\/tmp\/snippetselectdtest":$/;" resource line:32 language:PuppetManifest end:38
18 changes: 9 additions & 9 deletions Units/parser-puppetManifest.r/puppet-simpleselector.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@
file { "/tmp/snippetselectatest":
ensure => file,
mode => $var ? {
nottrue => 641,
value => 755
nottrue => "641",
value => "755"
}
}

file { "/tmp/snippetselectbtest":
ensure => file,
mode => $var ? {
nottrue => 644,
default => 755
nottrue => "644",
default => "755"
}
}

Expand All @@ -23,16 +23,16 @@
file { "/tmp/snippetselectctest":
ensure => file,
mode => $othervar ? {
"complex value" => 755,
default => 644
"complex value" => "755",
default => "644"
}
}
$anothervar = Yayness
$anothervar = "Yayness"

file { "/tmp/snippetselectdtest":
ensure => file,
mode => $anothervar ? {
Yayness => 755,
default => 644
"Yayness" => "755",
default => "644"
}
}
6 changes: 3 additions & 3 deletions Units/parser-puppetManifest.r/puppet-singleselector.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
$test = "yay"

$mode1 = $value1 ? {
"" => 755
"" => "755"
}

$mode2 = $value2 ? {
true => 755
true => "755"
}

$mode3 = $value3 ? {
default => 755
default => "755"
}

file { "/tmp/singleselector1": ensure => file, mode => $mode1 }
Expand Down
3 changes: 2 additions & 1 deletion Units/parser-puppetManifest.r/unless.d/expected.tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
/tmp/x input.pp /^ file { "\/tmp\/x": }$/;" resource line:2 language:PuppetManifest end:2
array input.pp /^$array = [ 3, 5, 7 ]$/;" variable line:1 language:PuppetManifest
/tmp/x input.pp /^ file { "\/tmp\/x": }$/;" resource line:3 language:PuppetManifest end:3
1 change: 1 addition & 0 deletions Units/parser-puppetManifest.r/unless.d/input.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
$array = [ 3, 5, 7 ]
unless $array[0] > 5 {
file { "/tmp/x": }
}
8 changes: 5 additions & 3 deletions circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,12 @@ jobs:
yum -y install git || :
- checkout
- run:
name: Install build tools
name: Install build and test tools
# TODO: enable spell checker
command: |
yum -y install gcc automake autoconf pkgconfig make libxml2-devel jansson-devel libyaml-devel findutils || :
rpm -Uvh https://yum.puppet.com/puppet5/puppet5-release-el-7.noarch.rpm \
&& yum -y install gcc automake autoconf pkgconfig make libxml2-devel \
jansson-devel libyaml-devel findutils puppet-agent-5.5.6-1.el7 || :
- run:
name: Build
command: |
Expand All @@ -48,7 +50,7 @@ jobs:
- run:
name: Test
command: |
make check roundtrip CIRCLECI=1
make verify-units-inputs check roundtrip CIRCLECI=1 PATH=/opt/puppetlabs/bin:$PATH

workflows:
version: 2
Expand Down
1 change: 1 addition & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ AC_ARG_PROGRAM
AM_CONDITIONAL(INSTALL_LIB, [test "x$enable_readlib" = "xyes"])
AM_CONDITIONAL(INSTALL_ETAGS, [test "x$enable_etags" = "xyes"])
AM_CONDITIONAL(USE_READCMD, [test "x$enable_readcmd" = "xyes"])
AM_CONDITIONAL(USING_BMAKE, [test "x$MAKE" = "xbmake"])

dnl AC_MSG_NOTICE(Change with $program_transform_name)
CTAGS_NAME_EXECUTABLE=`echo ctags | sed "$program_transform_name"`
Expand Down
27 changes: 26 additions & 1 deletion makefiles/testing.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# -*- makefile -*-
.PHONY: check units fuzz noise tmain tinst clean-units clean-tmain clean-gcov run-gcov codecheck cppcheck dicts cspell
.PHONY: check units fuzz noise tmain tinst clean-units clean-tmain clean-gcov run-gcov codecheck cppcheck dicts cspell verify-units-inputs

check: tmain units

Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ahakanbaba ahakanbaba Oct 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. One cannot use the make job control mode (-j) for verify-units-inputs. All input.XX file verification has to be done serially.
  2. 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.
  3. 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if !USING_BMAKE
# Find the test directories where ctags is expected to succeed. Only tests with
# an expected.tags file present are required to have valid input.
PUPPET_TEST_DIRS = $(foreach path, \
$(shell find $(srcdir)/Units/parser-puppetManifest.r -name expected.tags), \
$(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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not applicable anymore. Should be deleted

define VERIFY_ONE_PUPPET_TEST_DIR
$(1)/.input.pp.verified: $(1)/input.pp
puppet apply --noop $$< && \
touch $$@
endef

$(foreach puppet_test_dir,$(PUPPET_TEST_DIRS),$(eval $(call VERIFY_ONE_PUPPET_TEST_DIR,$(puppet_test_dir))))

#
# verify-units-inputs Target
#
verify-units-inputs: $(VERIFY_PUPPET_TEST_DIRS_TARGETS)

endif # if !USING_BMAKE

#
# UNITS Target
#
Expand Down