Don't show "open on github" link when we don't yet have anything to show#1697
Don't show "open on github" link when we don't yet have anything to show#1697robertbrignull merged 2 commits intomainfrom
Conversation
| } | ||
| } | ||
|
|
||
| private async getContextValue(element: QueryHistoryInfo): Promise<string> { |
There was a problem hiding this comment.
Can we extract this method out of the class so it can be tested separately? It doesn't seem like it uses this anywhere.
There was a problem hiding this comment.
You're right it's only getIconPath that relies on this, so we could pull out just getContextValue. I suppose doing it for one method is better than for zero methods.
There was a problem hiding this comment.
Ok, I've tried and I'm struggling quite a lot. The problem is that getContextValue depends on element.completedQuery?.query.hasInterpretedResults() and this calls into QueryEvaluationInfo and generally hits two problems:
The real implementation of the method relies on disk state, and the query history info tests aren't really set up for that. We could work around that passing in a helper method to getContextValue. It's a bit ugly but it would work. However there are deeper problems that than.
We don't do a good job of creating the right types in our tests. In this case createMockLocalQueryInfo doesn't actually create a LocalQueryInfo. It casts a random object to that type, but in reality LocalQueryInfo is a class so you can't just do this. This means when we call element.status it returns undefined because although element.t === 'local' it is not an instance of the LocalQueryInfo type, so the status method is not defined. I had a look at whether I could instantiate the class easily but it looks quite complicated. We do do some of this in query-history.test.ts but at this point things are getting very complicated and wide-ranging.
I'd like to merge this change as it is and then look at improving things separately.
There was a problem hiding this comment.
Fwiw, I'm going to introduce an improved version of the factory which does create a proper object. I need it in order to make sorting tests work. So you might be able to do this after that's merged.
One factory method to rule them all! There were a number of problems with these methods: 1. We were previously using two different factory methods to generate a fake local queries. Ideally we'd just have one. 2. We weren't really creating a real LocalQueryInfo object, which blocked us [1] from being able to correctly understand which fields we need in our tests and how they interact together. 3. We stubbed a bunch of methods on the original object to get our tests to work. We can now use a real object with all the trimmings. [1]: #1697 (comment)
We were trying to show the "Open variant analysis on github" link before we knew what the actions workflow run ID was, and therefore we were generating links like https://github.com/dsp-testing/qc-controller/actions/runs/undefined
See the individual commits on this PR. The first does some drive-by recfactoring and then the second commit actually implements the change in behaviour. I've tested it manually and it seems to be working as expected.
I went with just pulling
getIconPathandgetContextValueout to methods inquery-history.tsbecause they unfortunately have references to instance variables and to imports fromvscode. Otherwise I would have moved them toquery-history-info.ts, but maybe we can do that in the future. For this PR I just wanted to make an improvement and not get stuck on it.Checklist
ready-for-doc-reviewlabel there.