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

Rework vue-core #10712

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Rework vue-core #10712

merged 1 commit into from
Sep 11, 2024

Conversation

DamnClin
Copy link
Collaborator

No description provided.

@DamnClin DamnClin force-pushed the rework-vue-application branch 2 times, most recently from d9103ec to b5f5ca2 Compare August 31, 2024 18:36
@DamnClin
Copy link
Collaborator Author

The build is failing because I removed all tests because they were implementation details test. This is failing on npm run test:coverage don't find any test.

I don't know if I add a dummy test or change the target. Any input are welcome here :)

@DamnClin DamnClin force-pushed the rework-vue-application branch 2 times, most recently from b1fed55 to 57bf466 Compare September 1, 2024 05:56
@DamnClin DamnClin force-pushed the rework-vue-application branch from 57bf466 to da015ba Compare September 3, 2024 17:08
@@ -23,11 +23,6 @@ export default defineConfig({
cache: false,
include: ['src/test/webapp/unit/**/*.{test,spec}.?(c|m)[jt]s?(x)'],
coverage: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussion today: maybe replace this by threshold only for domain

Copy link
Collaborator Author

@DamnClin DamnClin Sep 3, 2024

Choose a reason for hiding this comment

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

Afterthoughts: not a good idea since some front domain models will just be types validated by component tests

Copy link
Member

@pascalgrimaud pascalgrimaud Sep 13, 2024

Choose a reason for hiding this comment

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

I'm a little bit late on this, we should improve this part, and only include what we want
I created this ticket #10822

@renanfranca
Copy link
Contributor

renanfranca commented Sep 6, 2024

I don't know if I add a dummy test or change the target. Any input are welcome here :)

@DamnClin : Could it be a home route test?

@renanfranca
Copy link
Contributor

@DamnClin : after this redesign got merged. Can you tell me if is it going to be a AxiosHttp module for each client (angular, vue, react)?

@DamnClin
Copy link
Collaborator Author

DamnClin commented Sep 7, 2024

I don't know if I add a dummy test or change the target. Any input are welcome here :)

@DamnClin : Could it be a home route test?

@renanfranca I'm strongly agains route test since this is an implementation detail test

@DamnClin
Copy link
Collaborator Author

DamnClin commented Sep 7, 2024

@DamnClin : after this redesign got merged. Can you tell me if is it going to be a AxiosHttp module for each client (angular, vue, react)?

Can be a good idea to have the same AxiosHttp module for every clients yes

@DamnClin DamnClin force-pushed the rework-vue-application branch from da015ba to 7ba73eb Compare September 7, 2024 13:21
@renanfranca
Copy link
Contributor

I don't know if I add a dummy test or change the target. Any input are welcome here :)

@DamnClin : Could it be a home route test?

@renanfranca I'm strongly agains route test since this is an implementation detail test

So dummy test is fine 🙂

@DamnClin DamnClin force-pushed the rework-vue-application branch 3 times, most recently from 4cd5cfc to 5217adf Compare September 9, 2024 07:01
@DamnClin
Copy link
Collaborator Author

DamnClin commented Sep 9, 2024

cc @Gnuk @murdos @pascalgrimaud what do you think of this version?

@DamnClin DamnClin force-pushed the rework-vue-application branch from 5217adf to 41fc434 Compare September 10, 2024 09:38
@renanfranca
Copy link
Contributor

renanfranca commented Sep 11, 2024

@DamnClin: I have already applied these changes to add the vue-core module to implement a sample that will be used to fix Vue: add OAuth2 keycloak authentication. Everything went well! Can you explain why you chose not to add the AxiosHttps stubs and tests? I think a well-implemented test would help guide those who use that module. Anyway, thank you for your work!

@DamnClin
Copy link
Collaborator Author

@DamnClin: I have already applied these changes to add the vue-core module to implement a sample that will be used to fix Vue: add OAuth2 keycloak authentication. Everything went well! Can you explain why you chose not to add the AxiosHttps stubs and tests? I think a well-implemented test would help guide those who use that module. Anyway, thank you for your work!

As it is, for me, this is testing an implementation detail

@DamnClin DamnClin merged commit d43cfff into jhipster:main Sep 11, 2024
34 checks passed
@DamnClin DamnClin deleted the rework-vue-application branch September 11, 2024 19:14
@pascalgrimaud pascalgrimaud added $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $500 https://www.jhipster.tech/bug-bounties/ labels Sep 14, 2024
@pascalgrimaud
Copy link
Member

I'm adding a big bounty on this, as it was a lot of work here.
Don't forget to claim it @DamnClin

@DamnClin
Copy link
Collaborator Author

I'm adding a big bounty on this, as it was a lot of work here. Don't forget to claim it @DamnClin

Thanks a lot, claimed #10712 (comment)

@pascalgrimaud
Copy link
Member

@DamnClin : the link is not correct :-D

@DamnClin
Copy link
Collaborator Author

@DamnClin : the link is not correct :-D

Sorry, misslick in github... https://opencollective.com/generator-jhipster/expenses/220095

@pascalgrimaud
Copy link
Member

@DamnClin : approved, thanks a lot for your work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: enhancement 🔧 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ client: vue theme: client $500 https://www.jhipster.tech/bug-bounties/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants