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

fix(i): Sort out invalid testing framework node indexing #3068

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Sep 25, 2024

Relevant issue(s)

Resolves #3065

Description

The main bug was only visible on sourcehub acp using http, due to the identity being copied with the audience value of another node's host failing authentication (the bearer tokens should be unique using correct node's audience). The biggest issue was the way we use getNodes and getNodeCollections. I would be in favor of completely removing them as they are more troublesome than the utility they provide.

Future:

How has this been tested?

  • Very painfully haha, had to install Linux bare-metal to investigate the first bug (sourcehub doesn't build on wsl for me) that was only occurring on sourcehub acp using http, due to the identity being copied with the audience value of another node the way we use getNodes and getNodeCollections

@shahzadlone shahzadlone added bug Something isn't working area/testing Related to any test or testing suite labels Sep 25, 2024
@shahzadlone shahzadlone added this to the DefraDB v0.14 milestone Sep 25, 2024
@shahzadlone shahzadlone self-assigned this Sep 25, 2024
@shahzadlone shahzadlone changed the title fix(i): Sort out the invalid testing framework node indexing fix(i): Sort out invalid testing framework node indexing Sep 25, 2024
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.36%. Comparing base (79eb12c) to head (6b9be21).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3068      +/-   ##
===========================================
+ Coverage    79.35%   79.36%   +0.01%     
===========================================
  Files          345      345              
  Lines        26557    26557              
===========================================
+ Hits         21074    21076       +2     
+ Misses        3956     3955       -1     
+ Partials      1527     1526       -1     
Flag Coverage Δ
all-tests 79.36% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79eb12c...6b9be21. Read the comment docs.

@shahzadlone shahzadlone force-pushed the fix/incorrect-node-index-http-sourcehub branch 3 times, most recently from 7b56f43 to 0e777fd Compare September 25, 2024 03:19
@shahzadlone shahzadlone requested a review from a team September 25, 2024 03:32
Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for documenting everything so well. I'm also in favor of removing the getNodes utilities.

@shahzadlone shahzadlone force-pushed the fix/incorrect-node-index-http-sourcehub branch from 0e777fd to 6b9be21 Compare September 26, 2024 07:05
@shahzadlone shahzadlone merged commit 20596e3 into sourcenetwork:develop Sep 26, 2024
41 of 43 checks passed
@shahzadlone shahzadlone deleted the fix/incorrect-node-index-http-sourcehub branch September 26, 2024 07:28
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Oct 20, 2024
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Oct 20, 2024
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Oct 20, 2024
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Related to any test or testing suite bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: consecutive source-hub acp operations fail for http client due to an identity auth error
2 participants