test addition for sync page - #502
Conversation
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
There's no single item here that I would block the merge over, but there are a number of small things (which seem to be centered around the name of the module 😛) which I think you should consider before merging, and I've got a bunch of other tweaks and suggestions, as well.
There was a problem hiding this comment.
As long as you're at it, do you want to have tests for the "/" and "/github" routes? The tests are trivial, but they improve the coverage and help prevent "rot". 😇
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
This looks excellent, Bala. There is just one item (other than my previous comment about trivial tests 😉) which I'd like you to consider.
But, if you're satisfied with both as they are, let me know and I'll close the conversations.
| _MODULE_PATH = str( | ||
| pathlib.Path(__file__).parent.parent / "sync-page" / "event-handler.py" | ||
| ) | ||
| sys.path.insert(0, str(pathlib.Path(__file__).parent.parent / "sync-page")) |
There was a problem hiding this comment.
This is an unsatisfying hack. Do you understand why it is necessary?
Could we get the same effect by omitting this from here and changing the Tox or unittest configuration? (None of the sync2jira tests have this...so why does the sync-page test need it?)
2 participants
Adding test case for sync page and also as per the feedback #501 (comment) calling the clean up job at requests..