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

Test rules #379

Merged
merged 11 commits into from
Mar 17, 2023
Merged

Test rules #379

merged 11 commits into from
Mar 17, 2023

Conversation

zcsahok
Copy link
Member

@zcsahok zcsahok commented Feb 19, 2023

To cover cases like #377 and also future contributed rules I propose to add tests for specific contest rules.
For each contest to be covered one has to set up a minimal logcfg+rule+log in a subdir of test/rules. The test starts in that directory TLF and expects that no changes happen to the log file. This ensures that score and mults are evaluated correctly.

First had to allow tests without referencing an object file.

Then the workflow has been improved to supply a valid datadir and to dump actual test failures.

@zcsahok
Copy link
Member Author

zcsahok commented Feb 19, 2023

Added a test for CQWW to show the idea.

And fixed build for Ubuntu 20.

@dl1jbe
Copy link
Member

dl1jbe commented Feb 19, 2023

Very good and helpful. Was just thinking about a way to test global rule handling here myself but missed time to implement it.

(My idea was to apply 'makelogline' with some given 'struct qso' sets, re-parse the line and compare the resulting structs.)

For your solution here are some points to think about:

  • Having a link to the original rule is good as the check is done against the actual rule set. But as far as I can tell automake does not support the distribution of links. A quick check (adding the rule to EXTRA_DIST in test/Makefile.am) showed that the TGZ file shows no link but a real copy of the rule file. That maybe acceptable as it is automatically generated from the original one.

  • The logfile.dat can not be a link but has to be different for any contest as it names the RULE= to use. But it may be a short dummy file with not much more in it.

  • We should expand the autoconf checks to having python and the pexpect module installed.

But anyway a good start to sort the problem out.

@dl1jbe
Copy link
Member

dl1jbe commented Feb 20, 2023

Just checked. A minimal logcfg.dat requires the following entries set:

  • RULES=
  • CALL=AN1CALL

@zcsahok
Copy link
Member Author

zcsahok commented Mar 6, 2023

Thanks for the feedback. Added check for pexpect. I have considered other options like using plain expect or adding a special test mode to TLF executable, but both have some drawbacks (installing tcl or not using the same code that the user does). configure.ac clash with #347 will be sorted out later.

Having symlinks resolved in distribution tar is fine, as it is a read-only copy anyway.

Will reduce logging and do some clean-up after the tests (#381).

@zcsahok
Copy link
Member Author

zcsahok commented Mar 6, 2023

Check for Python3 is too strict for Ubuntu 20, will rework it.

@dl1jbe
Copy link
Member

dl1jbe commented Mar 8, 2023

Thanks for the feedback. Added check for pexpect. I have considered other options like using plain expect or adding a special test mode to TLF executable, but both have some drawbacks (installing tcl or not using the same code that the user does).

'pexpect' should be fine. As we will use python anyway for #347 it seems a good choice.

Having symlinks resolved in distribution tar is fine, as it is a read-only copy anyway.

Yes, that should be no problems. It is also not much overhead with regards to the package size.

@zcsahok
Copy link
Member Author

zcsahok commented Mar 17, 2023

If the are no issues then this could be merged.

@dl1jbe
Copy link
Member

dl1jbe commented Mar 17, 2023

Very well. Just doing it. And Thanks

@dl1jbe dl1jbe merged commit cafdce5 into Tlf:master Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants