delph-in / matrix

The Grammar Matrix
https://matrix.ling.washington.edu/index.html
Other
12 stars 6 forks source link

Fixes delph-in#671 - bugfix for multiple inheritance for lexical types #673

Closed Ubadub closed 1 year ago

Ubadub commented 1 year ago

See discussion in #671

@ltxom let me know if anything is not satisfactory or if the regression tests run into issues. Thank you!

ltxom commented 1 year ago

Thank you. I verified these changes, which don't break other parts of the existing regression tests, and it solved the errors when validating choices_supertype_bugreport.txt.

@emilymbender Since it is a fix relating to the validation process, should we introduce a new regression test for it?

ltxom commented 1 year ago

Emily mentioned a unit test could be added to verify validation improvements. The section about how to run/add a unit test is here.

However, I followed the documentation above and found the parameter of "unit-test" is missing in the current Matrix trunk. I checked the commit history and found Dr. Goodman updated it to pytest.

Tests can be run with pytest. For now this requires setting the PYTHONPATH variable:

PYTHONPATH=. pytest tests/unit/choices_test.py
PYTHONPATH=. pytest tests/unit/validate_test.py

Don't just run all tests under tests/unit/ because some are still broken. The goal is to rework these tests under a more standard pytest workflow so simply running pytest will be enough.

@Ubadub Do you have experience using pytest and adding a unit test?

Ubadub commented 1 year ago

I tried running unit tests a couple days ago but ran into precisely the issue you have clarified above, so thank you for that. Yes, I've used pytest before though not in a few years but I'm sure I can figure it out. However, due to the demands of school it might be a few days before I can get around to it. I will keep you updated!

Ubadub commented 1 year ago

Also, as I mentioned in the Issue thread- there actually are regression tests for this, but for customization only, it seems nothing in the regression tests workflow tests the validation script (except indirectly). So maybe we want to change that too? I will write the unit test as soon as I can but if you want me to look into adding a few lines for having regression testing check validate.py as well, I can do that.

emilymbender commented 1 year ago

I'm confused as to the goal of this fix. It should be possible to do multiple inheritance on that page, I thought.

Ubadub commented 1 year ago

It isn't possible, specifically for lexical types. I've attached a choices file with issue #671 that will allow you to reproduce this issue.