Close #46: Add minimal acceptance tests and fix lint warning

[leandromoreira]

Close #46

In order to proceed with the lint fix, I introduced just two general acceptance tests using busted and running them in docker. I was afraid to break anything and these tests can help mitigate a little bit this fear, the idea is to adding up more use cases so when we do a change to the library we can test against it.

[leandromoreira]

Now one can also run:

make lint # to check lint mistakes
make test # to run the tests

[manoelcampos]

Hello @leandromoreira, I've just merged the PR. Everything worked great here. Thanks for that.

It would be nice to run the test on the CI using a GitHub Action. I'll do that if I make some time.

[manoelcampos]

Only after merging I realized some details and we could clean up a bit.

Makefile

I don't think this is required.

run:
    docker-compose down && docker-compose build nginx && docker-compose run --rm --service-ports nginx

Dockerfile.test

I didn't see the container using git, so, this command doesn't seem to be required. git config --global url."https://".insteadOf git:// \

[leandromoreira]

@manoelcampos thanks you're right I carried that over from my last lua project, about the build system we can use travis https://github.com/leandromoreira/resty-bakery but you must connect to travis and hook the project there and then it's simple as to add the https://github.com/leandromoreira/resty-bakery/blob/master/.travis.yml file

Maybe we need to remove the lint step since it might break the build =/ .

[manoelcampos]

Ok. I remove those lines. About the CI, I prefer GitHub Actions because they are easier to setup. I'll do that. Thanks.

[leandromoreira]

I think the tests can help us, I was wondering to discuss with you if we can change the table to string xml parser behavior. Today it expands a single node from <node attr="a" /> to <node attr="a">\n</node> is there any reason for that?

[manoelcampos]

No, there isn't. Probably it was the easiest way to implement that.

[manoelcampos]

By the way, it's tricky to check if after converting XML -> Lua -> XML, the result will be the same as the original XML. Check commit d536894 for details.

[manoelcampos]

In fact, I already had travis enabled. I just changed as you showed.

[manoelcampos]

I think the tests can help us, I was wondering to discuss with you if we can change the table to string xml parser behavior. Today it expands a single node from <node attr="a" /> to <node attr="a">\n</node> is there any reason for that?

About that, after parsing an XML to lua, we lose information if the tag is a regular or auto-closing one. This way, after converting back to XML, the only option is to generate them as regular tags.

[leandromoreira]

About that, after parsing an XML to lua, we lose information if the tag is a regular or auto-closing one. This way, after converting back to XML, the only option is to generate them as regular tags.

Are we able to check if the node/table member has value/children? if we can, we coud auto close any node without value/children

[leandromoreira]

local simple_lua_with_attributes = {
  {name="Manoela", city="Brasília-DF", _attr={ type="legal" }, music={_attr={like="true"}} },
  {name="Manoel", city="Palmas-TO", _attr={ type="natural" }, music={_attr={like="true"}} },
}

we can assume that given a member where its value is a table holding only _attr member can be declared auto closable, what do you think?

[leandromoreira]

I need to give you a heads up though, I'm advocating for that because my use case (mpeg-dash manifests) has single node to represent segments.

Also I noticed what seems like a bug, I guess, because the deeper it gets the more spaces it'll add, you can try to parse this into table and then back to xml to see what I'm tallking about.

To overcome some of these ^ I did some minimal transformations over the xml.

[manoelcampos]

Are we able to check if the node/table member has value/children? if we can, we coud auto close any node without value/children

Yes, we are, by just using #node the get the number of children.

local simple_lua_with_attributes = {
  {name="Manoela", city="Brasília-DF", _attr={ type="legal" }, music={_attr={like="true"}} },
  {name="Manoel", city="Palmas-TO", _attr={ type="natural" }, music={_attr={like="true"}} },
}

we can assume that given a member where its value is a table holding only _attr member can be declared auto closable, what do you think?

Makes sense, simplifying the generated XML. But if you want to ensure that when converting XML -> Lua -> XML, the resulting XML is equal to the original, it won't because some details are lost during conversion.

A node in the source XML may have only attributes and be declared as a regular tag anyway. After the round-trip conversion, it will be converted to auto-closing tag.

I need to give you a heads up though, I'm advocating for that because my use case (mpeg-dash manifests) has single node to represent segments.

Well, auto-closing tags are simpler and I don't see any issue assuming that.

Also I noticed what seems like a bug, I guess, because the deeper it gets the more spaces it'll add, you can try to parse this into table and then back to xml to see what I'm tallking about.

To overcome some of these ^ I did some minimal transformations over the xml.

This case I think is just about getting an organized XML, but we can improve the identation. I take a look at it.

[manoelcampos]

I couldn't see these double new lines you are talking about. Despite the missing <?xml>, result is almost the same as the original XML.