search

spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.75k stars 38.15k forks source link

Add test coverage for DomUtils #33768

Closed kunaljani1100 closed 3 weeks ago

kunaljani1100 commented 1 month ago

Unit tests have been added for the DomUtils class to test whether all the functions in this class work according to their expected behavior. Since testing for this class was not present tests have been added for this class.

In PR #33752, testing of DomUtils was done using Mocked objects, a modification has been done. Tests have loaded small XML files from the test classpath and assertion has been done on the actual Element.

kunaljani1100 commented 3 weeks ago

@snicoll, Can you please review this PR whenever you get a chance? This PR has been open for a while and there haven't been any comments left on my PR. Can you please let me know on what action I should be taking?

snicoll commented 3 weeks ago

This PR has been open for a while and there haven't been any comments left on my PR.

Sorry about that but we're a small team with limited time. This PR is not fixing a critical bug or isn't timely so it's on the low priority side of our bucket.

kunaljani1100 commented 3 weeks ago

This PR has been open for a while and there haven't been any comments left on my PR.

Sorry about that but we're a small team with limited time. This PR is not fixing a critical bug or isn't timely so it's on the low priority side of our bucket.

Sure, thanks a lot for letting me know about that and I understand that. I am a first time contributor who would like to explore open source and therefore I would like to contribute new features to spring.

snicoll commented 3 weeks ago

@kunaljani1100 thanks for your contribution. Please see https://github.com/spring-projects/spring-framework/commit/d43126705f9070c47274abafbd0baf8c8f7635b1 for the polish commit.

Going back to our previous discussion, unsolicited PRs (i.e. without an issue first to discuss the change) is OK but, as a small team, it needs to be in a good state and I am afraid we're not in a capacity right now to do the back and forth to reach that, which is why I polished it directly. Running the build would have revealed that JUnit assertions is not ok (or look at other tests if you need some inspiration).