Skip to content

Conversation

matthewnitschke-wk
Copy link
Contributor

@matthewnitschke-wk matthewnitschke-wk commented Oct 25, 2023

Added some rough snapshot tests for the diagnostics functionality that was recently added #98

When adding these, it was discovered that the reported diagnostics were not accurate, and there was a core bug in its implementation (using the wrong offset when finding which error was associated with the symbol). This bug was fixed in this PR as well

@aviary3-wk
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick and dirty solution to snapshots for diagnostic data. I'd prefer the source annotation approach that scip snapshot implements

Considering either contributing back to scip snapshot or just implementing a custom solution to this problem, but in the meantime this will let us track changes to diagnostics

SymbolMetadata getSymbolMetadata(
Element element,
AstNode node,
int offset,
Copy link
Contributor Author

@matthewnitschke-wk matthewnitschke-wk Oct 30, 2023

Choose a reason for hiding this comment

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

the specific node we pass in here isn't always the offset of the specific entity. For example, some elements are indexed by some analyzed element instead of the ast node at the specific depth

For this reason, just pass the offset as an int, so the errors can be correctly calculated where necessary

@matthewnitschke-wk matthewnitschke-wk changed the title added diagnostics snapshot test Added diagnostics snapshot test Oct 30, 2023
@matthewnitschke-wk matthewnitschke-wk changed the title Added diagnostics snapshot test Diagnostic bug fixes (and snapshot tests) Oct 30, 2023
@matthewnitschke-wk
Copy link
Contributor Author

QA +1

  • new diagnostic snapshot tests are technically missing a few results, but this is a huge improvement on the results that we are currently seeing

@matthewnitschke-wk
Copy link
Contributor Author

🚀 @Workiva/release-management-p 🚢

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rm-astro-wf rm-astro-wf merged commit c97eac8 into master Nov 2, 2023
@rm-astro-wf rm-astro-wf deleted the added_diagnostics_snapshot_test branch November 2, 2023 15:22
@matthewnitschke-wk
Copy link
Contributor Author

Closes #98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants