Skip to content

Conversation

@dyniols
Copy link
Contributor

@dyniols dyniols commented Oct 24, 2024

Based on the test plan #15509, this PR adds an e2e test checking memory granularities returned by get_mem_granularity.

@dyniols dyniols marked this pull request as ready for review October 25, 2024 07:44
@dyniols dyniols requested a review from a team as a code owner October 25, 2024 07:44
@dyniols dyniols requested a review from maarquitos14 October 25, 2024 07:44
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

Maybe I'm misreading the testplan, but it seems to me it says this test should be unittest not e2e test. @AlexeySachkov can you confirm?

@AlexeySachkov
Copy link
Contributor

Maybe I'm misreading the testplan, but it seems to me it says this test should be unittest not e2e test. @AlexeySachkov can you confirm?

Sorry for the confusion here. I should probably use a different terminology in the test plan, but due to the lack of a better one we have what we have. Suggestions are welcome, BTW!

Back to the question: even though the test plan says "unit", not all tests under that category are expected to be unit-tests in our regular understanding (i.e. put into sycl/test or sycl/unittests).

The main thing I wanted to highlight by the word "unit" is that we are not trying to test a proper meaningful usage scenario, but instead we just check internal consistency and correctness of certain APIs in isolation. That is done to have a better granularity in tests, so that when something breaks, we know the exact API which got broken and we don't have to debug a whole mini-application to understand why results are incorrect at the end.

This particular test should still stay E2E - moving it to sycl/unittest will mean that the only thing we would test are return values from mock plugin and not the actual values returned to us from low-level backends and the main intent here is to ensure that the values are meaningful. If we want to check that we have called the right UR APIs to implement those queries - that would be a sycl/unittest for sure and we can add that if necessary alongside with the one proposed here.

@maarquitos14
Copy link
Contributor

Thanks for the clarification, @AlexeySachkov, that makes sense.

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit.

@dyniols
Copy link
Contributor Author

dyniols commented Oct 28, 2024

@intel/llvm-gatekeepers I would appreciate if this PR could be merged.

@steffenlarsen steffenlarsen merged commit 7c509e9 into intel:sycl Oct 28, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants