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

WIP: Lwlock unit tests #5953

Closed

Conversation

joaopapereira
Copy link
Contributor

For now the PR is just a WIP because I still do not have the tests for the lwlock that I want. Feel free to take a look at this PR before it is ready as I would love to have some comments on the implementation of these Fakes.

What can you find in this PR:

Implementation of a Fake Test Double for the Shared Memory as well as some basic tests that will allow us to understand if the fake is behaving in the same way as the real implementation.
An example of sharing tests between both fake a real implementations.
Plumbing to replace the real implementation object with the fake ones
A PR that I sent to CMockery to add the prefix on the test name

@joaopapereira
Copy link
Contributor Author

@hlinnaka I created the branch in the wrong repository. So lets continue the conversation in this one:

Where are you going with this? What is the thing you actually want to test?

Going to try to walk y'll on my train of thought. Last week I was reviewing a PR that resulted from an interview and realized that it did not have unit tests. This was around the distributedlog functionality.
So I thought how can we test this functionality using our current unit test framework and realized that the best option would be if we have some Fakes that would help us do this. The first thing that poped up in my mind was the locking mechanism. If we have a fake for the locking mechanism we will not need to think anymore about it in our unit testing. So I decided that I wanted to start from there.

In order to do a Fake I would have to create some test around the LWLock to ensure that the it was
"quacking like a duck"(that the behavior of the same was the same as the real implementation without all the burden associated with setting up a Locking mechanism).
When I analyzed the code for the LWLock I realized that it relied heavily on the Shared Memory portion of postgres which made me think that I would have to go one deep and try to fake out the Shared Memory so that I could then fake out the LWLock so that I could have an easier time testing the distribute log.

And this train of thought brought me to the position where I am right now. The next steps that still need to be covered in this PR are:

Create a fake for the LWLock functions that we need distributed log
Use the LWLock and Share Memory Fakes to try to wrap some unit tests around the distributed log implementation
To be fair I am not 100% sure the second point will be part of this PR or if it makes more sense to be moved into its own PR, but time will tell.

Hope this helped out understand where I am coming from

@joaopapereira joaopapereira changed the base branch from lwlock-unit-tests to master October 9, 2018 18:34
@joaopapereira
Copy link
Contributor Author

Closed due to incorrect branch as a base

@joaopapereira joaopapereira reopened this Oct 9, 2018
@joaopapereira joaopapereira force-pushed the lwlock-unit-tests branch 5 times, most recently from e369720 to 907fb5d Compare October 11, 2018 15:59
@hlinnaka
Copy link
Contributor

Have you considered running the tests in a real server environment? You can write test C code e.g. in src/test/regress/regress_gp.c, and expose it as a SQL callable function.

@joaopapereira
Copy link
Contributor Author

joaopapereira commented Oct 11, 2018

Have you considered running the tests in a real server environment? You can write test C code e.g. in src/test/regress/regress_gp.c, and expose it as a SQL callable function.

I need a little bit more context to understand what you just said.
Are you suggesting the creation of function like test__LWLockAcquire_when_trying_to_acquire_101_locks_it_elogs_error that is run from a SQL file?
Or your suggestion is to expose to SQL the function LWLockAcquire and then in the SQL script we run it 100 times and check if it works?

joaopapereira and others added 4 commits October 11, 2018 16:33
If the user wants to run the same unit test multiple times
using the macro `unit_test_with_prefix` it is now possible
to differentiate between runs.

This macro appends to the function name what is passed into
the argument
example:
```
unit_test_with_prefix(my_prefix_, some_test_name);
```

will be translated into

`my_prefix_some_test_name`

Commit 45f7ec09307ea40b1e2c3d4ac18d3af30825e0b3 on google/cmockery
Add a folder of Helper functions for unit tests in the same directory
where the Mocks are stored

Signed-off-by: Joao Pereira <jdealmeidapereira@pivotal.io>
In this commit the infrastructure to add fakes is added.
Now we can create fake implementations of our objects in
`src/test/unit/test_doubles/path/to/file/realfilename_fake.c`

When we use them we need to add the following to the makefile:

```
shmem_fake.t: $(TEST_DOUBLE_DIR)/backend/storage/ipc/shmem_fake.o
```

This will include the fake instead of the real implementation of
the specific object.

As a proof of concept a shmem_fake was added with some tests that cover
both the fake and the real implementation.

---

Fakes are a different flavor of test doubles. Fakes and Mocks are Test
Doubles are used for different purposes.
The Fake object is usually used when we want the test double to mimics
the behavior of object that we are replacing. This new object will
respect the interface and will behave like the real object, but usually
is implemented in a much more simple way and should not be used in a
production environment.
An example of a common use of a Fake is a InMemoryDatabase.

In order to ensure that the Fakes are kept up to date we need to ensure
that the tests we write for the real implementation are also running
against the Fake, so that we are sure our Fake is still an acurate
representation of the real object.

The Mock object in the other hand functionally they are not smart. Their
objective is to ensure that when you call the test double the interface
is the same as the real object, but we control what is being returned at
each call, and in the end they check that the object was called with the
parameters that was expected.

As per the explanation above the cost of writting a Fake is higher then
writting a Mock in the short term. But in the long run if we need to use
the same Mock object over and over again we will realize that eventually
a Fake would have spared us some Test Code. In our example the Shared
Memory object is a object that will be used in multiple instances of our
tests and it make it easier to create a Fake that is able to replace the
Real object and maintain the behavior.
Added test for:
1. NumLWLocks
2. LWLockShmemSize
3. LWLockAcquire
4. LWLockRelease
6. LWLockHeldByMe
7. LWLockHeldExclusiveByMe

The first 2 tests are just to make sure we have enough coverage when we
create the fake of lwlock. It will be helpful to ensure that the values
returned from these functions is correct as well

In this patch a new `expected_elog_with_message` function was added to
the elog mocking helper to do the setup of the mock that ensure that
elog is called with the correct format.

This is where the Shared Memory fake starts to pay off. In this commit
we add more functionality to our Fake to allow us to retrieve some
"inner" information about the module itself. The functions
`shmem_fake_get_total_memory_allocated` and
`shmem_fake_retrieve_allocated_memory` allow us to look into the
amount of memory allocated by lwlock and also to return a pointer to
that shared memory.

We are using this trick to allow us to manipulate the lwlock module
information. This information is used to be able to test that a lock
can only be aquired if no other process as it. There is more
information about this in the comments of the functions that allow us
to have this behavior.
@hlinnaka
Copy link
Contributor

hlinnaka commented Oct 12, 2018 via email

@joaopapereira
Copy link
Contributor Author

Me and Jimmy tried and interesting project where we tried to port the tests for the merge repository. It can be found
https://github.com/joaopapereira/gpdb-postgres-
merge/commit/8de8095aae1bdcc1d44cfd4f6ef964d7c8c6c00e
joaopapereira/gpdb-postgres-merge@ba58e2f
And the interesting thing about it was that the changes that we had to make in order to port it is to change the calls to the public APIs.
So the tests are really not fragile, because if the changes are affected by the changes in public API there isn't much we can do but change the calls to them.

Despite that it looks like there is not a great deal of interest in having this change committed. So I will close this PR in the next couple of days

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.

None yet

3 participants