bowtie
bowtie copied to clipboard
bowtie-json-schema
Graphing trend on UI
@Julian
📚 Documentation preview 📚: https://bowtie-json-schema--1401.org.readthedocs.build/en/1401/
Hey @adwait-godbole, when raising a PR, it's always a good practice to write a short description of the changes/features/improvements you wanna achieve through the PR. This helps reviewers a lot by making it easier to understand the context at first glance. Can you do that for this one as well?
Hi @AgniveshChaubey this PR is a part of my GSoC project (bowtie trend). It introduces a visualization aspect using a line graph on the UI to better visualize a trend of how different versions of some particular implementation have differed in their failed tests, errored tests, etc. on different dialect test suites.
If you can run this on your local once it will surely give you a better understanding.
Meanwhile @Julian, can you please also have a look at this PR so I have something to show at https://bowtie.report.
Here's the demo:
https://github.com/user-attachments/assets/e05f9a6f-8a0f-491f-bede-8910fc628d12
Hey @adwait-godbole, the graph looks good in the first iteration already! I haven't had much time to look at the codebase thoroughly, but here are the few things I noticed at first glance...
- The dialect dropdown (at the top right side) is not visible in the dark mode.
- The version specific tooltip seems to have visibility issues in the dark mode due to faded texts.
One more thing... how are you collecting the version specific test results? The loading time for implementation-specific pages is very slow (nearly 5-7 seconds) in my case.
The dialect dropdown (at the top right side) is not visible in the dark mode. The version specific tooltip seems to have visibility issues in the dark mode due to faded texts.
I am yet to fix it in the dark mode 😅. For now, preoccupied with other stuff, will at least try to fix it soon enough.
One more thing... how are you collecting the version specific test results? The loading time for implementation-specific pages is very slow (nearly 5-7 seconds) in my case.
Well that's a complex process, the version specific test results are collected from such urls:
https://bowtie.report/implementations/python-jsonschema/v4.23.0/draft7.json https://bowtie.report/implementations/python-jsonschema/v4.22.0/draft7.json https://bowtie.report/implementations/python-jsonschema/v4.21.1/draft7.json https://bowtie.report/implementations/python-jsonschema/v4.21.0/draft7.json .. .. ..
etc. etc. and the implementations versions metadata is collected from: https://bowtie.report/implementations/python-jsonschema/matrix-versions.json
I have designed the frontend logic in such a way that when the user navigates to an implementation page let's say python-jsonschema then on the route async loader, it loads the very latest dialect's trend data (currently meaning 2020-12) and passes it on to the component. Then later only when the user selects the dropdown to change the dialect, only then that particular dialect's versioned reports data gets fetched and gets stored in a Map so we don't have to re-fetch it again when user navigates to some other dialect in the dropdown and comes back to it.
One more thing... how are you collecting the version specific test results? The loading time for implementation-specific pages is very slow (nearly 5-7 seconds) in my case.
Also not really sure why it's taking so long on your device. On mine it takes only roughly about 2-3 seconds or so. Maybe some internet speeds mismatch ig.
I have designed the frontend logic in such a way that when the user navigates to an implementation page let's say python-jsonschema then on the route async loader, it loads the very latest dialect's trend data (currently meaning 2020-12) and passes it on to the component. Then later only when the user selects the dropdown to change the dialect, only then that particular dialect's versioned reports data gets fetched and gets stored in a Map so we don't have to re-fetch it again when user navigates to some other dialect in the dropdown and comes back to it.
Now that I retrospect on this decision, ig I should just remove any async route loader and let the user first select a dialect from the dropdown and only then show the trend data. This will also reduce the implementation page's load time (the issue you were facing).
Will do this change by tomorrow.
- The dialect dropdown (at the top right side) is not visible in the dark mode.
- The version specific tooltip seems to have visibility issues in the dark mode due to faded texts.
Now that I retrospect on this decision, ig I should just remove any async route loader and let the user first select a dialect from the dropdown and only then show the trend data. This will also reduce the implementation page's load time (the issue you were facing). Will do this change by tomorrow.
@AgniveshChaubey @Julian fixed all of the above. Can we get this merged ?
@AgniveshChaubey @Julian here's a demo in both dark and light modes
https://github.com/user-attachments/assets/f886a867-fa5b-4141-941f-c93f015a2eb7
Hi @adwait-godbole, replying via email as I'm not on my PC right now. Will have a closer look at the code tonight. UI looks good now but instead of not showing anything initially, it'd be better to show the compliance trend for the latest dialect i.e., 2020-12
Hi @adwait-godbole, replying via email as I'm not on my PC right now. Will have a closer look at the code tonight. UI looks good now but instead of not showing anything initially, it'd be better to show the compliance trend for the latest dialect i.e., 2020-12
I did have it like this way! 🙂 until yesterday when you said that it was taking about 5-7 seconds on your machine to load. So I shifted to this design where we don't really load / fetch the data until the user himself doesn't explicitly select the dialect for which he wants to see the trend data for and now I think that this might be a better way since there is a lot of data to be loaded for showing the trend which unnecessarily makes the routing slower. Still would also like hear out @Julian's thoughts on this.
Not showing anything immediately doesn't look good to me. I didn't read super carefully but I wouldn't expect it to be slow to load all data? It's not that much data. It should be roughly the same amount of time as what you show there to load just 1 dialect as long as you do it in parallel -- and it will feel like even less, because the user will possibly have to scroll before they see the graph.
Not showing anything immediately doesn't look good to me. I didn't read super carefully but I wouldn't expect it to be slow to load all data? It's not that much data. It should be roughly the same amount of time as what you show there to load just 1 dialect as long as you do it in parallel -- and it will feel like even less, because the user will possibly have to scroll before they see the graph.
Cool. Okay. Will revert back to what I had.
Not showing anything immediately doesn't look good to me. I didn't read super carefully but I wouldn't expect it to be slow to load all data? It's not that much data. It should be roughly the same amount of time as what you show there to load just 1 dialect as long as you do it in parallel -- and it will feel like even less, because the user will possibly have to scroll before they see the graph.
Fixed this! @Julian
@AgniveshChaubey hope you find some time today to review this. Thanks.
Seems better now!
I saw you have used FC in many components to define the type of Props. But why import a new component (i.e., FC ) if you can directly specify the type of the prop just by doing this:
const DialectCompliance = ({implementation, dialectsCompliance} : Props) => {...}
After React 18, using FC is discouraged.
Check this for a better understanding.
Also, I found a lot of inconsistencies in code formatting and couple of snake case variables. In js we usually prefer camelCase variables. can you fix those as well?
I saw you have used FC in many components to define the type of Props. But why import a new component (i.e., FC ) if you can directly specify the type of the prop just by doing this:
const DialectCompliance = ({implementation, dialectsCompliance} : Props) => {...}
After React 18, using FC is discouraged. Check https://github.com/facebook/create-react-app/pull/8177 for a better understanding.
Fixed this!
Also, I found a lot of inconsistencies in code formatting and couple of snake case variables. In js we usually prefer camelCase variables. can you fix those as well?
Regarding the snake casing you're talking about, I could only find Dialect.newest_to_oldest() which I now fixed it to Dialect.newestToOldest(). Lmk what others you saw atleast in this PR for now.
@AgniveshChaubey Thanks!
HI Adwait, I have gone through the code and rest of the things seems fine to me. The only thing I am concerned about is the n^3 time complexity due to implementation.fetchVersionedReportsFor and prepareVersionsComplianceReportFor functions in the VersionsTrend.tsx component. Let me see if I can optimize it.
implementation.fetchVersionedReportsFor
I don't right away see this having an n^3 time complexity. Can you point out exactly how/where ?. All the JSON files are being fetched concurrently using Promise.all.
the versions.map(...) loop inside fetchVersionedReportsFor and the two nested loops inside prepareVersionsComplianceReportFor collectively makes the time complexity n^3.
the versions.map(...) loop inside fetchVersionedReportsFor and the two nested loops inside prepareVersionsComplianceReportFor collectively makes the time complexity n^3.
I just optimized it a bit. Can you have a look @AgniveshChaubey ?
I think it's possible to reduce the time complexity to the order of O(n). I'm working on it. I'll update you as soon as I find a way!
I think it's possible to reduce the time complexity to the order of O(n). I'm working on it. I'll update you as soon as I find a way!
Okay. Cool. While doing your optimizations, can you just make sure one thing that we don't change the return type of fetchVersionedReportsFor function i.e. Map<string, ReportData>. I am saying this because later on I also want to add diffing between two versions reports to see what results changed from one version to another version and not just the number of unsuccessfulTests. If you change it's return type to Map<string, Partial<Totals>> then later on will again have to change the code to return the actual entire ReportData and not just the Partial<Totals> corresponding to a version.
Okay. Cool. While doing your optimizations, can you just make sure one thing that we don't change the return type of fetchVersionedReportsFor function i.e. Map<string, ReportData>.
Sure, I'll take care of that.
Right now you have used fetchVersionedReportsFor function in the VersionsTrens.tsx component only, right?
Sure, I'll take care of that.
Right now you have used fetchVersionedReportsFor function in the VersionsTrens.tsx component only, right?
I just optimized it even further @AgniveshChaubey. Can you have a look? :)