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 eventio.ksy #1

Merged
merged 8 commits into from
Nov 18, 2023
Merged

Test eventio.ksy #1

merged 8 commits into from
Nov 18, 2023

Conversation

zonca
Copy link
Collaborator

@zonca zonca commented Nov 7, 2023

just to familiarize myself with the tools, I've ported most of the test_open_file.py unit test from pyeventio to the Kaitai version and used Github Actions to run it.

@zonca zonca self-assigned this Nov 7, 2023
test_open_file.py Outdated Show resolved Hide resolved
@zonca
Copy link
Collaborator Author

zonca commented Nov 7, 2023

Github action is running fine on my repository: https://github.com/zonca/eventio_kaitai/actions/runs/6788645920/job/18454092780

@zonca zonca requested a review from maxnoe November 7, 2023 18:25
@maxnoe
Copy link
Member

maxnoe commented Nov 8, 2023

@zonca I added you as developer to this repository, so in principle you don't need to fork.

@zonca
Copy link
Collaborator Author

zonca commented Nov 8, 2023

@maxnoe thanks, I added that function as a standalone function.
it would probably be nice to make it a method of Object, if I understand how to do it.
I think this PR is ready to merge unless you have feedback

# This workflow will install Python dependencies, run tests and lint with a single version of Python
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python

name: Python application
Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify away the not needed parts of this example? Also fill in better names?

E.g. rename python-app.yaml to python-tests.yaml

build -> CI or run tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do this later, when I'll do a python package, see #4

run: |
python -m pip install --upgrade pip
pip install flake8 pytest
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use requirements.txt but make a proper package using pyproject.toml and setuptools

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do this later, when I'll do a python package, see #4

# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
Copy link
Member

Choose a reason for hiding this comment

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

What do we do if we get linting errors on the auto-generated kaitai python module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

kaitai python module is generated later, I do not think we need to do linting on autogenerated code

- name: Compile Python module from definition
run: |
make py
- name: Test with pytest
Copy link
Member

@maxnoe maxnoe Nov 10, 2023

Choose a reason for hiding this comment

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

Since we discussed in #5 that we need to bundle the generated python code, I think we should have a test here that checks that the bundled module is up to date with the kaitai definition

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
kaitaistruct==0.10
Copy link
Member

Choose a reason for hiding this comment

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

See comment above, let's make a proper package and use pyproject.toml for dependencie specification

EventioKaitaiParser.from_file(TESTFILE)


def test_file_has_objects_at_expected_position():
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it, I think it is not necessary to test the addresses.

It should be good enough to check the expected types, versions and ids of all expected objects. There is basically no way to get a false positive here, if you got those correct, you have the file structure parsed correctly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is there any application that needs the addresses?
if we want to provide this as part of the interface, we should probably test it

Copy link
Member

Choose a reason for hiding this comment

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

No, it is just used for testing the Reader and debugging

@zonca
Copy link
Collaborator Author

zonca commented Nov 17, 2023

@maxnoe thanks for the feedback.

I wanted to do this small improvement over the current status of the repository and do a Python package for the next step, see #4. So I wouldn't bother with issues related to packaging and to the makefile right now.
However, if you prefer to have first a Python package, I can close this pull request and do another one when I have a full python package properly implemented.

@maxnoe
Copy link
Member

maxnoe commented Nov 17, 2023

I don't mind the order, if this already helps, let's go

@zonca
Copy link
Collaborator Author

zonca commented Nov 18, 2023

thanks @maxnoe, yes, I prefer to do smaller pull requests that gradually improve
I saved the important comment about lazy loading in #7.
Can you approve and merge this PR?

@zonca zonca requested a review from maxnoe November 18, 2023 04:24
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Please at least fix the makefile so it actually is correct, ie make test depend on the python module and use the proper output filename, so that make test works in a fresh checkout and it's only recompiled when there are changes.

improved makefile, by default compiles the parser
@zonca
Copy link
Collaborator Author

zonca commented Nov 18, 2023

@maxnoe ok done

@zonca zonca requested a review from maxnoe November 18, 2023 18:14
@maxnoe maxnoe merged commit 1fb7195 into cta-observatory:main Nov 18, 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.

3 participants