-
Notifications
You must be signed in to change notification settings - Fork 0
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
BI-46: Fetch University Data from MongoDB #22
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 3
Lines ? 26
Branches ? 4
========================================
Hits ? 26
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. |
src/__tests__/query.test.js
Outdated
expect(result?.data).toHaveProperty('universityList') | ||
expect(result?.data?.universityList).toBeInstanceOf(Array) | ||
expect(result?.data?.universityList?.length).toBeGreaterThan(0) | ||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the wrapping of try-catch from here.
src/__tests__/query.test.js
Outdated
// test to check the data is not empty | ||
test('the data is of college', async () => { | ||
try { | ||
const result = await fetchData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can directly fetch the result on this line, jest can handle the promises properly.
const result = await client.query({ query });
src/__tests__/query.test.js
Outdated
// test to check the format of data recived by the query | ||
test('the data has specific properties', async () => { | ||
try { | ||
const result = await fetchData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this test, you can again remove the try-catch wrapping and fetch the data directly.
} | ||
} | ||
// test to check the data is not empty | ||
test('the data is of college', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: If you want to try the describe statement, give it a shot.
You can fetch the data inside the describe and use the same variable inside both the tests. Here's an example (I copied it from ChatGPT):
describe('example describe block', () => {
let fetchedData;
beforeAll(async () => {
// Fetch data (e.g., from an API)
fetchedData = await fetchData();
});
test('test case 1', () => {
// Use fetchedData in the first test
expect(fetchedData).toBeDefined();
expect(fetchedData.length).toBeGreaterThan(0);
});
test('test case 2', () => {
// Reuse fetchedData in the second test
expect(fetchedData).toHaveProperty('property1');
expect(fetchedData).toHaveProperty('property2');
});
});
src/app/api/graphql/route.js
Outdated
try { | ||
const database = client.db('bacpac') | ||
const universities = database.collection('universities') | ||
const university = await universities.find().toArray() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename the variable to something that relates with an array or multiple universities.
A suggestion from my side: arrayOfUniversities
Try to recheck if we can eliminate find()
, if not, just leave it as it is.
src/app/api/graphql/route.js
Outdated
|
||
// The connection string for mongodb connection. | ||
const uri = process.env.MONGODB_URI || '' | ||
const uri = process.env.MONGODB_URI || 'mongodb+srv://bacpactech:[email protected]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's migrate the following keys to an env file. For GitHub we'll need it to add them as tokens. So, we'll have to recheck again if everything stays in sync.
src/app/api/graphql/route.js
Outdated
} | ||
` | ||
|
||
let plugins = [] | ||
const graphQLref = process.env.GRAPHQL_REF || '' | ||
const graphQLref = process.env.GRAPHQL_REF || 'bacpac-nbq1vs@current' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's migrate this one too to an env file.
src/app/page.js
Outdated
|
||
const query = gql` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make a file and import the query from there. Let's try to maintain a single file which contains all the queries.
src/client.js
Outdated
import fetch from 'cross-fetch' | ||
|
||
const client = new ApolloClient({ | ||
link: new HttpLink({ uri: 'http://localhost:3000/api/graphql', fetch }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss with @ayushtiwari110 about it. We'll keep public URL here.
const handleSearch = (e) => { | ||
let input = e.target.value.trim().toLowerCase() | ||
const filterData = searchAlgorithm(input) | ||
const filterData = searchAlgorithm(input, data).sort((a, b) => b.score - a.score) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit skeptical with the sort()
here. Maybe we can use it inside the search algorithm, in case that's the case, let's communicate with @ayushtiwari110 regarding this, we might need to write a new test for it, that'll drive this functionality.
if (input && data) { | ||
return matchSorter(data, input, { keys: ['name', 'country', 'city'] }) | ||
} else { | ||
return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I checked in the codecov, it turns out, this line wasn't tested at the moment. So, let's write a simple test for it, which can be triggered by empty data.
* changes made after review-1 * test error fixed * fixing testing error * fixing error in testing * fixing error of testing * fixing errors in testing query url * adding manual url * prev code to fix the error * deployed link added * Deployed link is added * vercal bot link added for test * after 2nd code review * switch statement is added for diffrent enviorments * absolute url in testing added * solving bi-46 issues * testing for apis * new link added * cors added * resolving error * test error resolved * prettier format checked * changes after 2nd review done
query for fetching data for searchbar is added, testing code will be added soon.