Contributions are welcome, but please help us review your changes (and keep code quality high) by following these guidelines. If you have any questions, feel free to ask. Don't let these guidelines be a barrier to contributing!
Code review: Changes should be as GitHub pull requests.
Use of GitHub issues: There should be at least one GitHub issue filed for your change (even trivial changes). This issue should explain the change, the impact on users, how you've tested it, and any other information that reviewers might need to know about the change. (Do not put this text in the commit message. See below.) If you're not sure exactly what change you want to make, feel free to create an issue to discuss it.
Commit messages: The commit message should consist of one line per associated issue, each line consisting of the issue number and issue synopsis. See previous commit messages for examples. There should be no other text in the commit message. Other information related to the change should generally be in the GitHub issue comments.
Completeness: Changes should include relevant updates to the documentation, including CHANGES.md. New commands, walkers, and non-private options must have associated documentation, both in the dmod (so that "::help DCMD" works) and in the usage guide.
Testing: All changes should be make prepush clean, but additional testing
is probably necessary for most changes. See below.
The appropriate level of testing depends on the scope of the change. Any non-trivial changes should be tested on:
- core files from each of the latest several major releases of Node (e.g., Node v4, Node v0.12, and Node v0.10)
- core files from both 32-bit and 64-bit programs
- core files generated by abort(3c) and core files generated by gcore(1M)
Depending on the change, you may want to test it manually on an existing collection of representative core files. If you're not sure about test requirements, please ask.
The automated tests are not a comprehensive regression test suite. Think of them more as a smoke test. You can run them by running:
make test(which is run bymake prepush)- the
catesttool, which lets you run individual tests separately - any of the individual tests by hand (they're standalone programs), which is likely easier for debugging the tests
All of these approaches use whichever Node is on your PATH to run a Node program and generate core files of that program. Your local build of mdb_v8 is used to inspect it.
For coverage across 32-bit and 64-bit and multiple Node versions, you can use
the runtests_node tool:
- First, run
runtests_node setup SOME_DIRECTORY, which downloads the supported Node versions into "SOME_DIRECTORY". - Then, run
runtests_node run SOME_DIRECTORY, which executes the full test suite on each of the versions in the directory.
Because we've made it pretty straightforward, we expect all changes to pass the
test suite on all of the versions and architectures used by runtests_node.
mdb runs with libumem, which means it's easy to check for memory leaks in C code. The process basically looks like this:
- Open up MDB on a core file and load mdb_v8.
- Exercise as much mdb_v8 functionality as you can (or whatever functionality you're trying to test for leaks.
- On platforms before illumos issue
6338 was fixed (which is all platforms
before 2015-10-26), run:
::unload libumem.so. This is the easiest way to work around that issue. - Use
gcoreto save a core file of your MDB process. - Open up that core file in MDB. (Yes, this gets confusing: you're using MDB to look at a core file of MDB looking at a core file of a Node program.)
- Run
findleaks -vto look for leaks.
There's a lot more information about using libumem to debug memory leaks elsewhere on the web.
There's a dumpjsobjects tool that takes a core file
and an mdb_v8 binary and uses findjsobjects and jsprint to enumerate all of
the objects in a deterministic order and print them out. With a representative
core file, this is a decent coverage test, since it will likely exercise the
paths for interpreting most types of supported objects. At the very least, this
should not crash.
For some kinds of changes, it's worthwhile to spend time comparing output from
before the change with output after the change to make sure you haven't
introduced any new issues. The mdbv8diff tool can be used to compare the
dumpjsobjects output from two different versions of mdb_v8.
Why not just use diff(1)? The challenge is that many changes to mdb_v8
change the output format slightly or fix nitty bugs that affect a large number
of objects. diff can easily produce hundreds of thousands of lines of output,
but there may be only a handful of different kinds of changes, and they may
all be easy to programmatically identify (e.g., the wording of a particular
error message changed). For this reason, mdbv8diff provides a small "ignore"
module interface, where you define a function that takes two lines of output
that look different and decides whether the difference corresponds exactly to an
expected change. Sometimes, a diff represents something that's hard to
programmatically recognize but you've manually confirmed that it's correct. You
can also add specific values as special cases to ignore. (These obviously
depend on the specific core file you're debugging.)
The intended workflow is that you run mdbv8diff on output from before and
after your change. It likely finds tons of diffs. You create a small "ignore"
module. As you go through the diffs one-by-one, if the diff represents a
regression, you fix your code. If the diff represents an expected change, you
teach your "ignore" module how to recognize it. You keep iterating this process
until mdbv8diff produces no more output.
This is obviously pretty crude. Think of this as a tool to assist the otherwise manual effort of exhaustively comparing lots of output, not an efficient regression tester.
The "ignore" modules for past changes are committed to the repo for reference, though they likely wouldn't be reused except to reproduce the original testing work for the change.