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

Fix GitHub actions and unit tests #29

Merged
merged 4 commits into from
Mar 24, 2024

Conversation

jthomperoo
Copy link
Contributor

Hi, I noticed the CI was failing, so I've attempted to fix that under commit f9ec13d.

Once I had the CI running again there were two unit tests failing (test_in_operator and simple_in_operator_for_string), which were both failing due to filtering by enums using the 'in' filter.

These look like they had been failing for a while, I couldn't pinpoint exactly when they started failing though, I was getting xUnit errors when trying to run the tests on older commits, probably something I haven't set up on my machine though.

The problem seemed to be when you filtered by an array of enums, e.g. birthmonth ^^ ["January", "March"], it recognised the type as an enum, but just tried to run the array string directly through Enum.Parse. I've updated the logic to detect if it is an array in the same way arrays of strings are detected in the FilterParser, and then individually parse each Enum entry. This looks like it fixes it, but still required the unit tests to be updated to reflect the new types. This was all done under commit 783b387.

Finally I just wanted to make sure I hadn't broken any functionality with any of this, so I've added some integration tests to cover some Enum scenarios under commit 662ff59. The tests compare enums, but also against a faked title just to avoid getting a whole load of other entries with matching enums, not sure if this is the best way to do it, happy to change it if there's a better approach.

These changes around Enums will probably cause conflicts with my other merge #28, so I'll have to update that if you accept this pull request.

Hope these changes all make sense!

Thanks.

@pdevito3
Copy link
Owner

pdevito3 commented Mar 5, 2024

thanks! i was going to check it out before i merged all in, but appreciate it :-)

@pdevito3
Copy link
Owner

pdevito3 commented Mar 5, 2024

yeah i just ran main locally and they all seem to be passing there fwiw! regardless, i'm sure it's just minor tweaks and i appreciate all the effort with this 🍻

@pdevito3 pdevito3 merged commit a9d7284 into pdevito3:main Mar 24, 2024
0 of 2 checks passed
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