Repository navigation
gitbug: Only query open issues - #1230
ryneeverett merged 3 commits into
Conversation
Exclude closed git-bug issues from sync query, mirroring behavior of other bugwarrior services. This means issues that are closed in git-bug will complete the corresponding tasks in taskwarrior. Filters for only open issues directly in GraphQL query so no closed issue should reach taskwarrior. Additionally adds small python condition to guard against git-bug upstream filter failures or query syntax drift. Adds tests for individual closed issue and mixed open and closed issue querying, and a query builder test to avoid accidental syntax regressions.
| def issues(self) -> Iterator[GitBugIssue]: | ||
| for issue in self.client.get_issues(): | ||
| # Guard against closed issues leaking through GraphQL query. | ||
| if issue.get("status") != "OPEN": |
There was a problem hiding this comment.
is this really needed, as the graphql query already filters on open issues, in get_issues ?
There was a problem hiding this comment.
Kept it as a future insurance if git-bug changes their graphql syntax/capabilities, but realistically that is very low change. Added commit removing that part of the PR.
There was a problem hiding this comment.
sidenote on that topic of API changes and payloads: @ryneeverett , I've been wondering on how to more easily test it (and maybe detect changes). Maybe the developer documentation should include a guide on how to set up each service to have an easy way to test it live. We could also store real examples of payloads somewhere in the tests, but we wouldn't be able to detect changes with that.
Obviously well beyond this MR scope
There was a problem hiding this comment.
I'm not sure I see the advantage of real payloads in tests. Assuming the service works at the time the tests are written, can't we conclude that the test data accurately represents the payload structure?
I'm open to it but I'm not sure documentation is the most effective way to live test services.
For hosted services, we could have a bugwarrior development account and run live tests in CI. To me this seems more useful than documentation because I'm way too lazy to create accounts for services I don't use.
For the few open source services like gitbug, we could have additional automated tests that actually run the service. Ideally a containerized solution that can run either locally or in CI.
There was a problem hiding this comment.
My point was: tests currently don't include raw payload from each services. So it's hard to refactor a service without calling the API (i.e. going through the whole account creation process, when we don't have one already).
As an example, for GitHub, the closest data we have from API response is the record fixture, but it's what the get_issues function would return, not the raw API. I'd like something like https://api.github.com/repos/GothenburgBitFactory/bugwarrior/issues/1230 stored somewhere, so we can ensure the service still works even if we heavily modify the implementation, as long as the tests pass.
GitHub is easy, we can access the API without any account, and I knew where to find this specific payload, but I don't want read each service implementation + their own documentation to understand where and how to retrieve that payload
We could continue this discussion in a new issue though, it does not belong here
There was a problem hiding this comment.
Ah, you're pointing out that coverage of parsing the raw payload into the record is missing from the tests. And that makes it hard to confidently refactor a service you don't use. Makes sense.
There was a problem hiding this comment.
GitHub is easy, we can access the API without any account, and I knew where to find this specific payload, but I don't want read each service implementation + their own documentation to understand where and how to retrieve that payload
As I recall, I added test for most of the services by making up the fake records based on which keys the service implementations access. I would think we could bootstrap the json payloads similarly and it should be a lot less tedious with an LLM.
We could also store real examples of payloads somewhere in the tests...
It isn't clear to me that there is any advantage to "real" payloads. The only distinctions I see are that they would contain keys we don't access and more realistic strings that we don't parse.
Since the git-bug graphql filter has seen no significant changes in syntax or features in the last ~7 years, we don't need the python guard. This commit drops the relevant code lines and instead relies on the GraphQL filter. Also removes accompanying tests for the guard.
|
@ryneeverett this PR is ready to be merged, right ? |
| (query,), _ = query_graphql.call_args | ||
| assert 'allBugs(query: "status:open")' in query | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't think this test is valuable. Tests should assert desirable behavior and not implementation details. In this case, I don't think there's a good way to regression test this bug at the moment.
- The first assertion is that a payload with no issues creates no tasks. That's not the bug you're fixing nor one that seems likely.
- The second assertion is that the query parameters you added to the
_query_graphqlargument are in fact in that argument. That's not the bug you're fixing nor one that seems likely.
There was a problem hiding this comment.
Reading the tests I wrote again, you're right that they only directly test the implementation on our side. At the time I did not find a better way to do this.
I could implement a similar responses-based construct (as e.g. the Github service issues test model does) which returns a different payload depending on query as per git-bug model.
But it would ultimately still need to gate on 'query: "status:open"',
so we'd still be testing an implementation detail.
For the moment I just removed existing query-builder tests,
and would wait for a decision on how to handle opensource services like git-bug instead per the conversation above.
The test asserts the literal query text rather than the behavior of the fix.
dd36105
into
GothenburgBitFactory:develop
Closes #1229.
Exclude closed git-bug issues from sync query, mirroring behavior of other bugwarrior services. This means issues that are closed in git-bug will complete the corresponding tasks in taskwarrior.
Filters for only open issues directly in GraphQL query so no closed issue should reach taskwarrior. Additionally adds small python condition to guard against git-bug upstream filter failures or query syntax drift.
Adds tests for individual closed issue and mixed open and closed issue querying, and a query builder test to avoid accidental syntax regressions.
Let me know if either the additional code-guard or the query builder test are unwanted, kept them as additional future insurance if upstream changes on their end or someone tries to change the query on ours.