Skip to content

Commit d9ea22b

Browse files
andygroveclaude
andauthored
docs: Add common pitfalls and improve PR checklist in development guide (#3231)
Add a new "Common Build and Test Pitfalls" section documenting: - Native code must be built before running JVM tests - Debug vs release mode (release only needed for benchmarking) - LD_LIBRARY_PATH requirement for libjvm.so when running Rust tests - Avoid using -pl to select modules (stale common module issue) - Use wildcardSuites for flexible test matching Restructure "Submitting a Pull Request" section as a numbered checklist: 1. Format code with `make format` 2. Build and verify with `make` 3. Run Clippy (recommended) 4. Run tests Add a quick Pre-PR Summary command reference. Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 0041bc5 commit d9ea22b

1 file changed

Lines changed: 114 additions & 2 deletions

File tree

docs/source/contributor-guide/development.md

Lines changed: 114 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,74 @@ A few common commands are specified in project's `Makefile`:
4848
such as Spark.
4949
- `make clean`: clean up the workspace
5050

51+
## Common Build and Test Pitfalls
52+
53+
### Native Code Must Be Built First
54+
55+
The native Rust code must be compiled before running JVM tests. If you skip this step, tests will
56+
fail because they cannot find the native library. Always run `make core` (or `cd native && cargo build`)
57+
before running Maven tests.
58+
59+
```sh
60+
# Correct order
61+
make core # Build native code first
62+
./mvnw test -Dsuites="..." # Then run JVM tests
63+
```
64+
65+
### Debug vs Release Mode
66+
67+
There is no need to use release mode (`make release`) during normal development. Debug builds
68+
are faster to compile and provide better error messages. Only use release mode when:
69+
70+
- Running benchmarks
71+
- Testing performance
72+
- Creating a release build for deployment
73+
74+
For regular development and testing, use `make` or `make core` which build in debug mode.
75+
76+
### Running Rust Tests
77+
78+
When running Rust tests directly with `cargo test`, the JVM library (`libjvm.so`) must be on
79+
your library path. Set the `LD_LIBRARY_PATH` environment variable to include your JDK's `lib/server`
80+
directory:
81+
82+
```sh
83+
# Find your libjvm.so location (example for typical JDK installation)
84+
export LD_LIBRARY_PATH=$JAVA_HOME/lib/server:$LD_LIBRARY_PATH
85+
86+
# Now you can run Rust tests
87+
cd native && cargo test
88+
```
89+
90+
Alternatively, use `make test-rust` which handles the JVM compilation dependency automatically.
91+
92+
### Avoid Using `-pl` to Select Modules
93+
94+
When running Maven tests, avoid using `-pl spark` to select only the spark module. This can cause
95+
Maven to pick up the `common` module from your local Maven repository instead of using the current
96+
codebase, leading to inconsistent test results:
97+
98+
```sh
99+
# Avoid this - may use stale common module from local repo
100+
./mvnw test -pl spark -Dsuites="..."
101+
102+
# Do this instead - builds and tests with current code
103+
./mvnw test -Dsuites="..."
104+
```
105+
106+
### Use `wildcardSuites` for Running Tests
107+
108+
When running specific test suites, use `wildcardSuites` instead of `suites` for more flexible
109+
matching. The `wildcardSuites` parameter allows partial matching of suite names:
110+
111+
```sh
112+
# Run all suites containing "CometCast"
113+
./mvnw test -DwildcardSuites="CometCast"
114+
115+
# Run specific suite with filter
116+
./mvnw test -Dsuites="org.apache.comet.CometCastSuite valid"
117+
```
118+
51119
## Development Environment
52120

53121
Comet is a multi-language project with native code written in Rust and JVM code written in Java and Scala.
@@ -161,10 +229,31 @@ It is possible to debug both native and JVM code concurrently as described in th
161229

162230
## Submitting a Pull Request
163231

232+
Before submitting a pull request, follow this checklist to ensure your changes are ready:
233+
234+
### 1. Format Your Code
235+
164236
Comet uses `cargo fmt`, [Scalafix](https://github.com/scalacenter/scalafix) and [Spotless](https://github.com/diffplug/spotless/tree/main/plugin-maven) to
165-
automatically format the code. Before submitting a pull request, you can simply run `make format` to format the code.
237+
automatically format the code. Run the following command to format all code:
238+
239+
```sh
240+
make format
241+
```
242+
243+
### 2. Build and Verify
244+
245+
After formatting, run a full build to ensure everything compiles correctly and generated
246+
documentation is up to date:
247+
248+
```sh
249+
make
250+
```
166251

167-
Additionally, it's strongly recommended to run Clippy locally to catch potential issues before the CI/CD pipeline does. You can run the same Clippy checks used in CI/CD with:
252+
This builds both native and JVM code. Fix any compilation errors before proceeding.
253+
254+
### 3. Run Clippy (Recommended)
255+
256+
It's strongly recommended to run Clippy locally to catch potential issues before the CI/CD pipeline does. You can run the same Clippy checks used in CI/CD with:
168257

169258
```bash
170259
cd native
@@ -173,6 +262,29 @@ cargo clippy --color=never --all-targets --workspace -- -D warnings
173262

174263
Make sure to resolve any Clippy warnings before submitting your pull request, as the CI/CD pipeline will fail if warnings are present.
175264

265+
### 4. Run Tests
266+
267+
Run the relevant tests for your changes:
268+
269+
```sh
270+
# Run all tests
271+
make test
272+
273+
# Or run only Rust tests
274+
make test-rust
275+
276+
# Or run only JVM tests (native must be built first)
277+
make test-jvm
278+
```
279+
280+
### Pre-PR Summary
281+
282+
```sh
283+
make format # Format code
284+
make # Build everything and update generated docs
285+
make test # Run tests (optional but recommended)
286+
```
287+
176288
## How to format `.md` document
177289

178290
We are using `prettier` to format `.md` files.

0 commit comments

Comments
 (0)