sinProject-Inc / talk

Listening and Speaking

https://sinproject.net

MIT License

3 stars 6 forks source link

Set tsconfig to strictest #1445 #1446

Closed REI-MORI closed 9 months ago

REI-MORI commented 10 months ago

closes #1445

For the Submitter

Tasks

[x] Make sure all checkboxes in the Issue content are checked
[x] Make sure the PR title is in the format <Issue Title> <Issue Number>
[x] Make sure "closes (issue number)" is written close to the top
[x] Attach the enhancement label for update tasks
[x] Make sure CI completes successfully
[x] Check everything written in the PR
[x] Fix the issues pointed out by AI
[x] Add reviewers
[x] On Slack, mention the PR reviewer, write your thoughts on the issue, and inform them that the PR has been created

For the Reviewer

Checks

[x] Each task for the related issue are written and in checkboxes
[x] The branch name is close to the issue name
[x] CI completes successfully
[x] Changes include only what is necessary for this PR
[x] Necessary unit tests are implemented
[x] Necessary E2E tests are implemented
[x] Perform functionality checks
[x] If there are any problems, request revisions from the PR submitter and mention them on Slack

After Reviewing

[x] Leave an approval comment (e.g. LGTM)
[x] On Slack, mention the PR submitter and a project admin, write which PR you have reviewed, and ask them to merge the PR
[x] Write an approval comment with praise on Slack

github-actions[bot] commented 10 months ago

Update Summary

Several small changes were made to improve the type safety and access of certain properties.

src/lib/components/menu_items_sub.svelte

The way a user object is accessed from $page.data has been changed.

src/routes/(authed)/translate/+page.svelte

A condition was added to return the function if an undefined object is encountered in the texts array.

src/routes/+layout.svelte

The theme property of the html.dataset is accessed in a safer way.

Suggestions for improvement

src/lib/components/menu_items_sub.svelte

Please ensure that user property always exists in $page.data, which can make the code cleaner.

src/routes/(authed)/translate/+page.svelte

The function could perhaps throw an exception or log a warning if it gets an undefined text.

src/routes/+layout.svelte

The two blocks of code that update the html.dataset.theme could potentially be extracted to a helper function to reduce duplication.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

In the recent commits, the handling of ValidText instances has been refactored in multiple files.

src/lib/general/text_error.ts

Refactoring has removed instantiation of a new ValidText instance. Instead, a static method ValidText.validate(text) is being used to validate the input.

src/lib/text/valid_text.ts

A new static method validate(text) has been added. This method creates an instance of ValidText and returns the validated text.

src/lib/view/message.ts

Same refactoring as in text_error.ts: usage of ValidText.validate(text) instead of creating a new ValidText instance.

Suggestions for improvement

src/lib/text/valid_text.ts

Renaming the validate(text) function to something more descriptive would better convey its operation, such as createValidTextInstance(text).
Adding a check before creating a new instance in validate(text) can avoid unnecessary object creation if the text is already valid.

src/lib/general/text_error.ts, src/lib/view/message.ts

As ValidText.validate(text) is being used in both the constructors, it could be better to create a common method in a parent class or utility function, which can be called in both these places. This would ensure DRY (Don't Repeat Yourself) principle and easier future modifications.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

This revision centers around updates and enhancements in the tests for the ClientHostName class in client_hostname.test.ts. Here are the precise changes:

src/lib/network/client_hostname.test.ts

Mocked the dns module, simulating a scenario where a valid IP address ('8.8.8.8') resolves to a given hostname ('dns.google') and invalid IP addresses lead to an 'getHostByAddr EINVAL' error.
Imported vi from 'vitest' and used it to perform the mock operation.
In the 'should return the hostname for a valid IP address' test, the same test scenario has been maintained.
Improved the 'should return an empty string for an invalid IP address' test. Previously, it was just expecting the reverse method to return an empty string for invalid IP addresses. Now, it also takes into account the possible throwing of an error, catching it and setting hostname to an empty string when an error happens.

Suggestions for improvement

src/lib/network/client_hostname.test.ts

Ensure the IP address '8.8.8.8' and the hostname 'dns.google' are not hard-coded throughout the test file but defined as constants at the top. This allows for easier updates in future tests.
Consider breaking down the logic of mocking dns into simpler parts or small helper functions to improve readability.
It would be helpful to add more test cases with even more varying scenarios.
The error handling in the 'should return an empty string for an invalid IP address' test could be improved by asserting that the error message is as expected. This would ensure that the error caught is the one we anticipate and not just any accidental error.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

The changes involve refactoring the test code in the text_error.test.ts and valid_text.test.ts files in order to more efficiently and dynamically validate different test cases. This is done by creating an array of test cases, each consisting of an input and its expected output or error, and running each case in a loop. The different tests that were previously done one by one have now been condensed into this more dynamic testing approach.

src/lib/general/text_error.test.ts

Individual test cases composed of input and expected output/error messages created and stored in a test_cases array
A loop added to traverse the test_cases array, running the specified test for each test case. Error checks and message comparisons are included within the loop.

src/lib/text/valid_text.test.ts

Similar to the previous file, individual test cases are created and stored in an array
A loop added to traverse the test_cases array, executing specified tests for each case - error checks, text comparisons, and text validation are included within the loop.

Suggestions for improvement

src/lib/general/text_error.test.ts

Consider adding more diverse and exhaustive test cases in the test_cases array to cover more potential edge cases.
Additional checks may be added in the test loop to validate other properties or behaviours of the TextError class.

src/lib/text/valid_text.test.ts

Extended test coverage could be achieved by adding further diverse test cases in the test_cases array to cover more potential edge cases.
Additional checks may be added in the test loop to validate other aspects of the ValidText class, such as whether it handles special characters as expected.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

The update replaced some null and undefined checking mechanisms in three files.

translate_with_google_advanced.ts

The checking if the translations exists and if the translations array is empty was unified in one line. The same result of returning an empty string when conditions are not met are retained.

+page.server.ts

Similar to the first file, the checking of the return of the split function and ensuring there is data on the second index was unified in one line.

+server.ts

Lastly, the speech_sounds array check was changed akin to what's been done on the first file ensuring that the error message is thrown when the length of the array is 0.

Suggestions for improvement

translate_with_google_advanced.ts

While the changes made were okay to shorten code length, it would be better for readability if an error message is also thrown just like what's done on the +server.ts when the conditions for the translations are not met.

+page.server.ts

It would be better to have a function that checks the validity of the image_type before updating the user's avatar to prevent the occurrence of unnecessary errors.

+server.ts

The error message of 'speech_sounds is undefined' can be misleading because even if the array is defined but empty, the error will still be thrown. Therefore, a more accurate error message can be 'speech_sounds array is empty'.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

The code changes in these files are primarily related to refactoring of test cases for readability and reuse. The changes mainly consist of the extraction of repetitive bits of code into reusable parts, and restructuring the test cases to use Jest's parameterized test format.

`background.test.ts`

Added a type declaration for Test specs which consists of a name, index, expected, and error_message.
Instead of testing individual cases, a specs[] array of test cases was created.
Parameterized tests were introduced using test.each() function that iterates through specs[] and perform same set of operations on each element.

`text_error.test.ts`

The refactoring has included a new naming and structuring for the parameterized test cases, for better readability.
In test cases, "expected_message_id" is replaced with "expected" for simpler and clear comprehension.

`client_hostname.test.ts`

Converted two tests into a single parameterized test to cover all scenarios.

`valid_text.test.ts`

Adopted a similar parameterized testing approach here as was done in the former.

`message.test.ts`

Similar to the other test files, The test cases were restructured to use Jest's parameterized test format.

Suggestions for improvement

The tests are quite comprehensive and the refactoring seems solid. They are readable and easy to understand. However, here are some minor improvements that might be considered.

`background.test.ts`

Consider adding a comment explaining what each test in the Specs array is specifically testing for. This will help new developers understand the test plan more quickly.
The index, expected, and error_message properties in the Spec type could use a more descriptive naming to reflect their purpose in the test.

`text_error.test.ts`

As for the other test files, it may be helpful to add a inline comments to each test case in the parameterized test to explicitly state what the test case is intending to check.

`client_hostname.test.ts`

The items in the Spec[] array can have a description property to know what is being tested in each test case.

`valid_text.test.ts`

For easier readability and understandability, variables could be renamed ("input", "expected_message", "expected_error") to reflect their roles in the test more accurately.

`message.test.ts`

Similar to above, consider introducing descriptive names for variables in specs.
For negative tests (expected errors), consider separating them out into their own parameterized test for improved clarity and organization.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

In this update, changes have been made mainly to test files and configuration files.

.github/pull_request_template.md

Modified instructions related to merging PR on Slack, with an added step for project admins.

.github/workflows/ci.yml

Modified the GitHub Action configuration to include fetching full commit history when checking out the repository.
Added SonarCloud Scan configuration for running static code analysis on the application.

src/lib/*/.test.ts

Renamed the test functions from "test" operator to "it".

vite.config.ts

Implemented configuration for generating coverage reports and specified files to exclude from coverage.

Suggestions for improvement

.github/pull_request_template.md

Instructions may be made more detailed or broken down into smaller steps if necessary, depending on the team's familiarity with the git workflow.

.github/workflows/ci.yml

Consider version pining of the third-party actions to avoid unexpected breaking changes or malpractices.

src/lib/*/.test.ts

Optimization of tests and adding more edge cases can be considered for increased code coverage and trustworthiness.

vite.config.ts

Review the excluded files in the coverage report configuration and ensure that no important files or directories are missed.

by OpenAI

github-actions[bot] commented 10 months ago

Update Summary

There are updates in two files: '.github/workflows/ci.yml' and 'vite.config.ts'.

.github/workflows/ci.yml

Renamed the workflow Vitest Test to Vitest Test and coverage.
Change the command for running the Vitest test to npx vitest run --coverage from npm run test:ci.

vite.config.ts

Changed the path of 'create_git_branch.ts' from 'src/lib/git/create_git_branch.ts' to 'src/scripts/create_git_branch.ts'.

Suggestions for improvement

.github/workflows/ci.yml

Place comments above complex commands to describe their purpose for better readability.
Validate if other npm run commands can be replaced with npx to streamline dependencies.

vite.config.ts

Ensure the new path 'src/scripts/create_git_branch.ts' correctly points to the desired file and scripts directory holds similar script files for consistency.
Add comments around the change to explain why the path is changed (if this was not a simple refactor).

by OpenAI