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

add testing for normal search #2927

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Conversation

JiaHua-Zou
Copy link
Contributor

Issue This PR Addresses

#2621

Type of Change

  • Tests: Add testing for the normal search function.
  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Adding tests for the normal search function.
The test has cover all of the normal search function.
image

Steps to test the PR

pnpm install
pnpm test search

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@JiaHua-Zou JiaHua-Zou requested a review from RC-Lee February 14, 2022 19:59
@gitpod-io
Copy link

gitpod-io bot commented Feb 14, 2022

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Do you want to do the same thing here with the version so it can get the mock stuff from Satellite?

path: '/posts/post/_search',
},
() => {
return { statusCode: 503 };
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned in my tests as well, it's not the statusCode that is being returned by ElasticSearch that is returning the 503 error.
It's failing because Search is trying to do these two lines here in the return.
It's trying to read hits.total and hits.hits, but the hits object is undefined here because its not being returned by ElasticSearch.
For example, changing it to return { statusCode: 404 } will also pass this test.
I think we might need to rephrase the description for this test, so its not as misleading

Copy link
Contributor Author

@JiaHua-Zou JiaHua-Zou Feb 17, 2022

Choose a reason for hiding this comment

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

ok. I will rephrase the description

@@ -1 +1 @@
process.env = { ...process.env, MOCK_ELASTIC: '1' };
process.env = { ...process.env, MOCK_ELASTIC: '1', LOG_LEVEL: 'error' };
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't add the LOG_LEVEL in my PR, but commented that anyone could do this locally.
I guess its up for discussion if we wanted to keep this or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciated the LOG_LEVEL hint in your PR. My console was flooded with stuff until I changed that variable

Copy link
Contributor

Choose a reason for hiding this comment

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

What I usually do is add a console.log() to say, we have our logs on silent, but you can change in file x/y/z to error or debug if you want, and then make this silent, or it spams too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I think that is a good idea. Even in error mode. It still print a whole bunch of stuff.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Great stuff.

@humphd humphd merged commit 724c5ec into Seneca-CDOT:master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: test Creation and development of test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants