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

Event Handler Infrastructure #22

Merged
merged 17 commits into from
Jan 10, 2025
Merged

Event Handler Infrastructure #22

merged 17 commits into from
Jan 10, 2025

Conversation

Alomir
Copy link
Collaborator

@Alomir Alomir commented Dec 18, 2024

This PR adds infrastructure to SIPNET to enable handling of agronomic events.

Structure

  • Use a compile-time flag to turn this functionality on/off
  • Put this flag (and similar future flags) in a separate file to allow for easier testing (by having only one file that needs to be replaced to control tested functionality, and also to have nothing else in that file)
  • Create unit test(s)
  • Update the github workflow to run the new tests

Design

  • Read event information from a specified file, one line per event.
  • The spec for the agronomic events is here; a reviewer with access to that doc should make sure this code follows that spec.
  • Store event information in a similar fashion as climate data
  • Future (next step, really): augment sipnet.c to fetch the appropriate events at a given location in the now-empty setupEvents function

Testing

  • This is an attempt (first try?) at adding some infrastructure for unit testing.
  • Basic idea:
    • Unit tests go in a subdirectory of /tests/sipnet (or another sub of /tests/, if we add specific testing for, say, estimate)
    • Create a model_structures.h file with the correct compile switches for the functionality to be tested
    • Copy /tests/Makefile.template and add the test c-files to it
    • ...and write the tests, of course. The infrastructure should handle the rest.
    • See tests/sipnet/test_events for an example

Notes

  • frontend.c: it was tempting to simplify this code by removing the #if checks for EVENT_
    HANDLER, but that seemed out of character for SIPNET
  • sipnet.c: I commented out some variable declarations to clean up compiler warnings

@Alomir Alomir marked this pull request as ready for review January 9, 2025 17:42

eventType = getEventType(eventTypeStr);
if (eventType == UNKNOWN_EVENT) {
printf("Error: unknown event type %s\n", eventTypeStr);
Copy link
Member

Choose a reason for hiding this comment

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

I'm am generally unfamiliar with C but learning - is this line intended to be similar to the check on line 87?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is - and it's possibly redundant. Line 87 catches bad data in the file, and 135 here catches a bad return from getEventType. I think right now there's no likely way to to get past the line 87 check and hit the 135 check, but that might not be true in the future.

Copy link
Member

@dlebauer dlebauer left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution, it is exciting to have the project under way. Although my experience with C is limited, this looks like a great start that provides a solid foundation. 🎉

Great work with the comprehensive testing - I did not previously appreciate the complexity of testing in C! Also with checks for a valid events file and helpful corrective guidance.

With respect to migrating compile time flags to a separate file - is this a relatively straightforward cut and paste? If not / if it would help, I can provide a list of flags that are higher priority since we will be changing from the defaults.

Sorry I made some minor changes (mostly to Makefile and ci.yml after mergeing #21 and then resolving merge conflicts); these can be seen here: 0d8d16c...93adfd1

Not urgent, but if you document how to add a test it could help to delegate some of the testing (I haven't taken the time to understand how this works, but it looks daunting).

@Alomir
Copy link
Collaborator Author

Alomir commented Jan 10, 2025

I agree, we need some docs for adding tests. They are a bit daunting - some of that is due to this being C, but also needing to recompile to enable functionality really brings it up a notch. I hope I've hidden most of that, time will tell 🤞

@Alomir Alomir merged commit becec4d into master Jan 10, 2025
6 checks passed
@dlebauer dlebauer deleted the event_handler_infrastructure branch January 10, 2025 19:04
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