Well, why do you need the ipnr? I always thought nobody was actually interested in that and thought that we should even remove it in later releases.

As I said, I'm writing a plugin which shows the wiki data to the user as a table and I just wanted to be as comprehensive as possible. If you think about this that way I will just drop it. It's not an important thing for the plugin.

What about a hacked trac and where people upload unwanted attachments or alter wiki pages in ways we cannot imagine? Wouldn't it be nice to at least have the ip address from where the alteration or malicious upload came from?

db42.py makes me think we could start to introduce a DDL helper functions, like upgrade_table(old_Table, new_Table) and a Table.remove_columns that would create a new Table instance, so that one could write upgrade_table(attachment_table.remove_columns('ipnr'), attachment_table)

show_ip_addresses was removed in r14909. That change should probably have been committed with these changes.

db42.py makes me think we could start to introduce a DDL helper functions, like upgrade_table(old_Table, new_Table) and a Table.remove_columns that would create a new Table instance, so that one could write upgrade_table(attachment_table.remove_columns('ipnr'), attachment_table)

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

Is this only needed if the column with auto_increment has changed, or has been removed? If the column with auto_increment has not changed, then that column will have the same data in the new table as in the old.

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

Is this only needed if the column with auto_increment has changed, or has been removed? If the column with auto_increment has not changed, then that column will have the same data in the new table as in the old.

The upgrade_tables re-create tables. Auto-increment column uses sequence object in PostgreSQL. Then, the sequence object would be re-created and started with 1. Inserting old records wouldn't change the sequence.

It might be worth considering to put the logic in DatabaseManager.destroy_db()​. I suppose a test would be useful to prove the case, but from looking at the code, wouldn't the logic be needed if there was a reason to destroy an on-disk SQLite database outside of EnvironmentStub?

Running unit tests with r15037 while investigating the failure, the EnvironmentStub.reset_db_and_disk() accidentally removed trac in my source directory. EnvironmentStub.path is set to os.path.dirname(trac.__file__) if path argument is empty at tags/trac-1.0.12/trac/test.py@:360-365#L332​.

>>> env=EnvironmentStub()>>> env.path'/src/tracdev/git/trac'

I think we should check whether self.path is in current directory to avoid the accident before removing self.path directory.

I've encountered that problem as well. It might not be an issue, but an environment created in the root of the source directory would not be removed.

The reason I started thinking about that was remembering that the functional tests, by default, create the environment in <path to source>/testenv/trac: trunk/trac/tests/functional/__init__.py@14794:133-138#L122​. The functional tests don't call reset_db_and_disk, so your patch doesn't create a problem in this case.

It's probably bad practice to create an environment directory in the source tree, so I think the patch is okay. I have two thoughts on the functional test cases though:

Should we destroy the environment at the end of the test run?

Should we put the environment in a temp directory? If putting in a temp directory, we might want to randomize the directory name.

Also, adding a mkdir function in trac.test, we could ensure use of the same prefix for all temporary directories.

Should we put the environment in a temp directory? If putting in a temp directory, we might want to randomize the directory name.

When an exception is raised from twill, the html is saved as <test-case-name>.html in log directory of the test environment. I think it would be better to keep the environment if tests are failing, at least.

I consider the following code in [808e806f/rjollos.git] would remove all owned files in the temporary directory if the path of EnvironmentStub.__init__ is None and reset_db_and_disk() is wrongly called in tearDown().

If unit tests run with on-disk SQLite file, we generally use TRAC_TEST_DB_URI=sqlite:test.db. With the branch, /tmp/test.db would be created (currently, trac/test.db is created in source tree). Then, only unit tests could run in the same time on system.