Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1286 closed defect (fixed)

C++ test code should be built by 'make'

Reported by: jinmei Owned by: jreed
Priority: medium Milestone: Sprint-20111025
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

As a side effect of #1091, we now need to do 'make check' to compile
C++ test code. For developers this is very inconvenient; in incremental
development derived from TDD, we often make small adjustments to the code
(either the main one or test code) and begin testing by actually compiling
it (sometimes confirming it does *not* compile is part of test). Running
the entire test everytime we do this is a huge overhead.

We should revert to the previous behavior: just typing 'make' should
build both the main code and test code.

I know there was alredy an exception to this behavior in the log module.
I've not looked into it to see whether it's a mistake or a necessary
compromise due to (e.g.) dependencies that cannot be resolved easily
otherwise. If it's the former, it would be better to fix it at this
opportunity.

Note: how to solve this issue is up to whoever takes this task, but it
would also be nicer if we clarify the use of check_ and noinst_ prefixes
in Makefile.am. #1091 introduces many "check_" rules for the purpose of
not installing test related libraries, but it also caused this side effect.
For consistency, it's better to have a unified usage that achieves both

  • not to install test-only libraries
  • allow test code to be build by 'make' (not requiring 'make check')

It would aslo be better to be documented in the coding guideline page.

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jinmei

  • Summary changed from C++ code should be built by 'make' to C++ test code should be built by 'make'

comment:2 Changed 8 years ago by jreed

  • Owner changed from UnAssigned to jreed
  • Status changed from new to assigned

comment:3 Changed 8 years ago by jreed

  • Owner changed from jreed to UnAssigned
  • Status changed from assigned to reviewing

see commit b126cf8 (trac1286 branch).

comment:4 Changed 8 years ago by jreed

In my commit message I said log depended on datasrc, that is due to isc/init_.py that imports it.

comment:5 follow-up: Changed 8 years ago by jinmei

Not looking at the diff, but some test related libraries seem to
be installed: libfake_session, libtestutils.

Anyway, this ticket is not in the current sprint. I suggest we handle
this in the next sprint.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jreed

Replying to jinmei:

Not looking at the diff, but some test related libraries seem to
be installed: libfake_session, libtestutils.

Those were already there. I guess they should have been handled in the #1091 ticket. I can extend this ticket to also fix that too.

comment:7 in reply to: ↑ 6 Changed 8 years ago by jreed

Replying to jreed:

Those were already there. I guess they should have been handled in the #1091 ticket. I can extend this ticket to also fix that too.

Done in commit 0b6ac7e

comment:8 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111025

comment:9 follow-up: Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:10 in reply to: ↑ 9 Changed 8 years ago by jinmei

Replying to jinmei:

It basically looks okay. I also noticed we now don't do post-make
compile for logging related tests, which is nice, too.

One last thing: we probably don't need to install libbench. It's
not intended for general purpose use, and only our internal micro
benchmark tools need it.

Feel free to make a change on this point (which should be trivial
and wouldn't need another round of review) and merge.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jreed

comment:12 Changed 8 years ago by jreed

  • Resolution set to fixed
  • Status changed from reviewing to closed

libbench is now not installed.

branch merged

Last edited 8 years ago by jreed (previous) (diff)

comment:13 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3
Note: See TracTickets for help on using tickets.