Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tempo: Add } when { is inserted automatically #80113

Conversation

harrymaurya05
Copy link
Contributor

@harrymaurya05 harrymaurya05 commented Jan 7, 2024

Which issue(s) does this PR fix?:
Adding } when { is inserted automatically

Fixes #74027

screen-capture.online-video-cutter.com.1.mp4

@harrymaurya05 harrymaurya05 requested a review from a team as a code owner January 7, 2024 18:10
@CLAassistant
Copy link

CLAassistant commented Jan 7, 2024

CLA assistant check
All committers have signed the CLA.

@grafana-pr-automation grafana-pr-automation bot added datasource/Tempo area/frontend pr/external This PR is from external contributor labels Jan 7, 2024
@fabrizio-grafana fabrizio-grafana changed the title traceql : adding } when { is inserted automatically Tempo : Add } when { is inserted automatically Jan 7, 2024
@fabrizio-grafana fabrizio-grafana added this to the 10.3.x milestone Jan 7, 2024
@fabrizio-grafana fabrizio-grafana changed the title Tempo : Add } when { is inserted automatically Tempo: Add } when { is inserted automatically Jan 7, 2024
@fabrizio-grafana
Copy link
Contributor

fabrizio-grafana commented Jan 7, 2024

Great job! The code seems fine. In the next days I'll also do some quick manual testing as a double check. If everything works fine, we can then merge 🚀

Copy link
Contributor

@fabrizio-grafana fabrizio-grafana left a comment

Choose a reason for hiding this comment

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

Well done! Thank you!
It just seems some tests fail locally. Could you check and possibly fix them?

Copy link
Contributor

@fabrizio-grafana fabrizio-grafana left a comment

Choose a reason for hiding this comment

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

This is the test suite that seems to fail: public/app/plugins/datasource/tempo/traceql/autocomplete.test.ts

@harrymaurya05
Copy link
Contributor Author

Well done! Thank you! It just seems some tests fail locally. Could you check and possibly fix them?

Thanks for highlighting the failing test suite in autocomplete.test.ts. I'll investigate the issue and work on resolving the test failures. I'll provide updates as soon as I have more information or when the issue is resolved.

@harrymaurya05
Copy link
Contributor Author

harrymaurya05 commented Jan 9, 2024

Hi @fabrizio-grafana
Issue identified:
Test case are not updated as per new change for empty input. now fixes are added in current changes please review and provide you valuable inputs.

yarn test summary:
2 test case are still failing but its not related to current changes. you can correct me if i am missing something.

Summary of all failing tests
FAIL public/app/features/explore/spec/split.test.tsx (70.239 s)
● Handles open/close splits and related events in UI and URL › handles split size events and sets relevant variables

thrown: "Exceeded timeout of 30000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  180 |   });
  181 |
> 182 |   it('handles split size events and sets relevant variables', async () => {
      |   ^
  183 |     setupExplore();
  184 |
  185 |     const splitButton = await screen.findByText(/split/i);

  at public/app/features/explore/spec/split.test.tsx:182:3
  at Object.<anonymous> (public/app/features/explore/spec/split.test.tsx:38:1)

FAIL public/app/features/explore/spec/queryHistory.test.tsx (139.943 s)
● Explore: Query History › updates the state in both Explore panes › deleted queries are synced

thrown: "Exceeded timeout of 30000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  141 |
  142 |   describe('updates the state in both Explore panes', () => {
> 143 |     beforeEach(async () => {
      |     ^
  144 |       const urlParams = {
  145 |         left: serializeStateToUrlParam({
  146 |           datasource: 'loki',

  at public/app/features/explore/spec/queryHistory.test.tsx:143:5
  at public/app/features/explore/spec/queryHistory.test.tsx:142:3
  at Object.<anonymous> (public/app/features/explore/spec/queryHistory.test.tsx:81:1)

Test Suites: 2 failed, 1 skipped, 1278 passed, 1280 of 1281 total
Tests: 2 failed, 27 skipped, 11610 passed, 11639 total
Snapshots: 334 passed, 334 total
Time: 557.762 s
Ran all test suites.

@fabrizio-grafana
Copy link
Contributor

Yeah those might be flaky tests, they should not be affected by changes in that part of the codebase. I tried locally to run those tests and I had no error, so I think we are good to go!

@fabrizio-grafana
Copy link
Contributor

fabrizio-grafana commented Jan 11, 2024

@harrymaurya05 it seems the CI linting check is failing. Can you run yarn run prettier:check and push the changes? Thanks!

@harrymaurya05
Copy link
Contributor Author

harrymaurya05 commented Jan 11, 2024

@harrymaurya05 it seems the CI linting check is failing. Can you run yarn run prettier:check and push the changes? Thanks!

Hi @fabrizio-grafana
I found out the issue and fixed as well. now pushing new changes.

Harioms-MacBook-Air:grafana hariommaurya$ yarn run prettier:check

[warn] public/app/plugins/datasource/tempo/traceql/autocomplete.test.ts
[warn] public/app/plugins/datasource/tempo/traceql/autocomplete.ts
[warn] Code style issues found in 2 files. Run Prettier to fix

Fix code style issue reported by prettier.
yarn run prettier --write "public/app/plugins/datasource/tempo/traceql/autocomplete.test.ts" "public/app/plugins/datasource/tempo/traceql/autocomplete.ts"

@fabrizio-grafana fabrizio-grafana merged commit 5c8e88d into grafana:main Jan 11, 2024
12 checks passed
@fabrizio-grafana
Copy link
Contributor

Amazing job! Merged!

@harrymaurya05
Copy link
Contributor Author

Amazing job! Merged!

Hello @fabrizio-grafana ,

Thank you so much for your kind words and for merging the pull request! This is my first even contribution in open source project. I really appreciate your guidance and support throughout this process. I've learned a many things by working on this issue.
Looking forward to continuing to contribute more in grafana. Thanks again!

Best regards,
Hariom Maurya @harrymaurya05

@ifrost ifrost modified the milestones: 10.3.x, 10.4.x Jan 12, 2024
s0lesurviv0r pushed a commit to s0lesurviv0r/grafana that referenced this pull request Feb 3, 2024
@aangelisc aangelisc removed this from the 10.4.x milestone Mar 6, 2024
@aangelisc aangelisc added this to the 10.4.0 milestone Mar 6, 2024
@harrymaurya05 harrymaurya05 deleted the add-missing-curlyBracket-on-autoComplete branch April 3, 2024 17:27
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.

Missing } when { is inserted automatically
5 participants