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.

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

Resolve an issue with "=" delimeter, change previous namings #2541

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@alexsaber
Copy link

commented Mar 26, 2019

No description provided.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Thank you for the further fix! Please make sure to read the output of the build server. Release notes are missing.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@alexsaber

This comment has been minimized.

Copy link
Author

commented Mar 27, 2019

Thank you for your feedback. Yes, I see, some tests are failing. For example, "key = something" expected, but "key=something" received.
Where are these tests defined? I am sorry, I have almost no experience with C/C++... If you point me where the tests are, I can fix them of course. Thanks.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Most of the test cases are in src/plugins/ini/testmod_ini.c and tests/shell, one test case seem to be in src/plugins/blockresolver/testmod_blockresolver.c

- infos = Information about the ini plugin is in keys below
- infos/author = Thomas Waser <thomas.waser@libelektra.org>
- infos/licence = BSD
- infos=Information about the ini plugin is in keys below

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 29, 2019

Member

I do not think that it is a good idea to remove these spaces in the plugin contract, since the function generate_readme seems to require them.

@alexsaber

This comment has been minimized.

Copy link
Author

commented Mar 30, 2019

@sanssecours Thanks for your comment. I changed it back.
@markus2330 I am sorry I bother you again. I see testmod_blockresolver causes problems again.
This line fails
succeed_if (compare_line_files (srcdir_file (compareName), foutname), "files do not match as expected");
What does this check exactly do? If I try to trace it back, it still does not make sense to me why my changes broke this test. I could find neither documentation nor comments inside in code to get a better understanding of this use case. Can you please help me out?

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

First of all, please make sure to always add a line to the release notes before creating a PR. Otherwise the the Jenkins builds don't run and you will just waste time. You can always change the line later, if you need to.

srcdir_file constructs the path to a file in the source directory of the plugin. And elektraFilename() returns the name of a temporary file. So the line question actually compares the temporary file created by the kdbSet call with the file src/plugins/blockresolver/blockresolver/compare.block.

The Travis log indicates, that you probably changed the output of the plugin, so either revert that change or change the compare.block file to match the expected output.


PS. you can link to code snippets instead of copying the line:
https://help.github.com/en/articles/creating-a-permanent-link-to-a-code-snippet

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

There are still some tests failing, because of the whitespace arround =. You are also still missing the line in the release notes.

@alexsaber

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

@kodebach thanks for your comment! Do you mean the tests are failing because I changed the behavior it interprets the whitespaces around '=' or because somewhere in the test data there are still the whitespaces? If it is the second one, I cannot find anything written in the old format anymore. I change all the test data to the new format, meaning I removed the whitespaces.
I will add the line in the release notes, thank you.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Lets take for example this Travis job:

As you can see in Line 1881 the testshell_markdown_base64 test fails:

141/226 Test  #44: testshell_markdown_base64 ................***Failed    1.09 sec

The following lines show you the output. Line 1929-1932 for example tell you one of the problems that occurred:

CMD: kdb file user/tests/base64 | xargs cat
RET: 0
STDOUT: binary="@BASE64AGLuy2N/AAA="
=== FAILED stdout does not match expected pattern binary = "@BASE64[a-zA-Z0-9+/]+={0,2}"

The command kdb file user/tests/base64 | xargs cat was expected to produce output matching the regex binary = "@BASE64[a-zA-Z0-9+/]+={0,2}", but instead produced binary="@BASE64AGLuy2N/AAA=". As you can see, the problem the test expected spaces arround the =, but there weren't any.

If you don't know, the kdb file command outputs the path of the file in which a key is stored. So the whole kdb file user/tests/base64 | xargs cat simply prints the file containing the key user/tests/base64.

@alexsaber

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

Thank you for you comment!
I do understand that the test expected to get @BASE64[a-zA-Z0-9+/]+={0,2}, but got @BASE64AGLuy2N/AAA= instead.
I tried to change @BASE64[a-zA-Z0-9+/]+={0,2} to @BASE64[a-zA-Z0-9+/]*={0,2}, but it didn't help. So how to make this test to not expect whitespaces around =? I changed the way the ini plugin works with the whitespaces around = and I tripple-checked if I missed something. In my opinion, it should work.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

You read the regex wrong. The problem is not the @BASE64[a-zA-Z0-9+/]+={0,2} part. That is fine, and it matches @BASE64AGLuy2N/AAA=. The problem is that it also expects binary = at the start, but it got binary=. So you need to change binary = "@BASE64[a-zA-Z0-9+/]+={0,2}" into binary="@BASE64[a-zA-Z0-9+/]+={0,2}".

The problems with other tests are similar. Just look for an = () and try replacing it by = ().

alexsaber added some commits Apr 7, 2019

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

It seems there are some formatting problems... Please reformat the files in question, so that the build server can do its job

@alexsaber

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

Thank you for your comment!
Ok, I see that testscr_check_formatting is failing. You are mentioning reformatiing of some files. Which files do you mean? I haven't changed the files this test if using. I think so.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

The file that fails the check is src/plugins/ini/README.md, but you can always just run all of the reformat-* scripts (or if you rebase the new reformat-all script). The reformatting script won't do any damage if everything is perfect already.

@sanssecours

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Which files do you mean?

As you can see here, the files

  • doc/INSTALL.md and
  • src/plugins/ini/README.md

are not formatted correctly. You can use the description in the link above to fix this problem.

@alexsaber

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

I see. Thank you very much for your support!

@alexsaber

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

Ok, I still need to work on formatting...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.