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: custom dns-resolvers test passes #55

Closed
wants to merge 7 commits into from

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Apr 17, 2024

Title

fix: custom dns-resolvers test passes

Description

temporary fix for failing tests with dns-resolvers when using sessions: #50 (comment)

This should not be occurring and we should ensure this works with sessions.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

achingbrain and others added 5 commits April 11, 2024 15:27
Adds a configurable session cache that creates sessions based on
the base URL of the requested resource.

E.g. `https://Qmfoo.ipfs.gateway.com/foo.txt` and`https://Qmfoo.ipfs.gateway.com/bar.txt`
will be loaded from the same session.

Defaults to 100 sessions maximum with a TTL of one minute. These
are arbitrary numbers that will require some tweaking.
@SgtPooki SgtPooki requested a review from a team as a code owner April 17, 2024 21:16
Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

@SgtPooki SgtPooki changed the title fix: custom dns-resolvers test passes when sessions=false fix: custom dns-resolvers test passes Apr 17, 2024
@achingbrain
Copy link
Member

achingbrain commented Apr 23, 2024

Just a thought but as implemented the tests fail because we expect configured gateways to fail to find the block, but now the session will try to find it in the routing.

In this test we could ensure that the routing is not consulted by resolving the DNSLink record to an identity CID instead?

This will likely make the test less fragile going forwards.

diff --git a/packages/verified-fetch/test/custom-dns-resolvers.spec.ts b/packages/verified-fetch/test/custom-dns-resolvers.spec.ts
index 9d330f8..f8ff641 100644
--- a/packages/verified-fetch/test/custom-dns-resolvers.spec.ts
+++ b/packages/verified-fetch/test/custom-dns-resolvers.spec.ts
@@ -21,7 +21,7 @@ describe('custom dns-resolvers', () => {
   it('is used when passed to createVerifiedFetch', async () => {
     const customDnsResolver = Sinon.stub().withArgs('_dnslink.some-non-cached-domain.com').resolves({
       Answer: [{
-        data: 'dnslink=/ipfs/QmVP2ip92jQuMDezVSzQBWDqWFbp9nyCHNQSiciRauPLDg'
+        data: 'dnslink=/ipfs/bafkqac3imvwgy3zao5xxe3de'
       }]
     })
 
@@ -30,8 +30,9 @@ describe('custom dns-resolvers', () => {
       dnsResolvers: [customDnsResolver]
     })
     const response = await fetch('ipns://some-non-cached-domain.com')
-    expect(response.status).to.equal(502)
-    expect(response.statusText).to.equal('Bad Gateway')
+    expect(response.status).to.equal(200)
+    expect(response.statusText).to.equal('OK')
+    await expect(response.text()).to.eventually.equal('hello world')
 
     expect(customDnsResolver.callCount).to.equal(1)
     expect(customDnsResolver.getCall(0).args).to.deep.equal(['_dnslink.some-non-cached-domain.com', {
@@ -44,7 +45,7 @@ describe('custom dns-resolvers', () => {
   it('is used when passed to VerifiedFetch', async () => {
     const customDnsResolver = Sinon.stub().withArgs('_dnslink.some-non-cached-domain2.com').resolves({
       Answer: [{
-        data: 'dnslink=/ipfs/QmVP2ip92jQuMDezVSzQBWDqWFbp9nyCHNQSiciRauPLDg'
+        data: 'dnslink=/ipfs/bafkqac3imvwgy3zao5xxe3de'
       }]
     })
 
@@ -62,8 +63,9 @@ describe('custom dns-resolvers', () => {
     })
 
     const response = await verifiedFetch.fetch('ipns://some-non-cached-domain2.com')
-    expect(response.status).to.equal(502)
-    expect(response.statusText).to.equal('Bad Gateway')
+    expect(response.status).to.equal(200)
+    expect(response.statusText).to.equal('OK')
+    await expect(response.text()).to.eventually.equal('hello world')
 
     expect(customDnsResolver.callCount).to.equal(1)
     expect(customDnsResolver.getCall(0).args).to.deep.equal(['_dnslink.some-non-cached-domain2.com', {

Base automatically changed from feat/use-blockstore-sessions to main May 9, 2024 17:57
@SgtPooki
Copy link
Member Author

SgtPooki commented May 9, 2024

this was resolved in #50

@SgtPooki SgtPooki closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants